[DeivceASAN] Make ShadowMemory one instance per type

[yingcong-wu]

UR PR: oneapi-src/unified-runtime#2585

[zhaomaosu]

lgtm

[yingcong-wu]

Kindly ping @unified-runtime-reviewers for review.

[kbenzie]

@yingcong-wu please take a look at the Jenkins/Precommit. I already retried it once but haven't checked if the second run had any more info than the first.

[yingcong-wu]

Hi @kbenzie , this looks like infrastructure issue at first glance. I will do more investigation.
Also, this PR is not so vital as the UR part. The UR part can function as expected without this PR.

[AllanZyne]

I suggest move this test to "VirtualMem" folder, because if it is failed, owners of "VirtualMem" will take responsible for it.

[yingcong-wu]

I suggest move this test to "VirtualMem" folder, because if it is failed, SYCL runtime team will take responsible for it.

I don't think that is ideal because I think they don't care about cross context access as that may be a violation to the spec. Ideally this test should be written using level-zero API, but that may be not so convenient. We can update this test later and add more tests to test for other assumption we made for the L0/GPU.

[AllanZyne]

I suggest move this test to "VirtualMem" folder, because if it is failed, SYCL runtime team will take responsible for it.

I don't think that is ideal because I think they don't care about cross context access as that may be a violation to the spec.

Since this is our requirement, we need to communicate with them to support this feature.
But "VirutalMem" related issue is not our sanitizer team's responsibility.
Anyway, we need them aware this requirement.

[AllanZyne]

We may create a "sanitizer" folder under "VirtualMem" to let them know this is specific to sanitizer.

[yingcong-wu]

I suggest move this test to "VirtualMem" folder, because if it is failed, SYCL runtime team will take responsible for it.

I don't think that is ideal because I think they don't care about cross context access as that may be a violation to the spec.

Since this is our requirement, we need to communicate with them to support this feature. But "VirutalMem" related issue is not our sanitizer team's responsibility. Anyway, we need them aware this requirement.

If this becomes a requirement, then the requirement should be made to L0/GPU team instead of SYCL team.

[AllanZyne]

I suggest move this test to "VirtualMem" folder, because if it is failed, SYCL runtime team will take responsible for it.

I don't think that is ideal because I think they don't care about cross context access as that may be a violation to the spec.

Since this is our requirement, we need to communicate with them to support this feature. But "VirutalMem" related issue is not our sanitizer team's responsibility. Anyway, we need them aware this requirement.

If this becomes a requirement, then the requirement should be made to L0/GPU team instead of SYCL team.

Yep, I updated my words, it's responsibility of owners of "VirutalMem" folder, not "AddressSaniitzer".

[yingcong-wu]

Also, this is not a hard requirement for the whole DASAN, but just a requirement for the current implementation of shadow memory. If it does not support cross-context access one day, we can adjust our implementation for shadow memory. Also, it is hard to make GPU team commit to that requirement because in theory, we should not do cross-context access.

[yingcong-wu]

I suggest move this test to "VirtualMem" folder, because if it is failed, SYCL runtime team will take responsible for it.

I don't think that is ideal because I think they don't care about cross context access as that may be a violation to the spec.

Since this is our requirement, we need to communicate with them to support this feature. But "VirutalMem" related issue is not our sanitizer team's responsibility. Anyway, we need them aware this requirement.

If this becomes a requirement, then the requirement should be made to L0/GPU team instead of SYCL team.

Yep, I updated my words, it's responsibility of owners of "VirutalMem" folder, not "AddressSaniitzer".

I still think we should keep this test in ASAN folder, because 1: this is not really a legitimate test for VirtualMem(cross-context), and 2: this would help us catch the problem sooner, rather than wait for the issue to be dispatched.

[AllanZyne]

Also, this is not a hard requirement for the whole DASAN, but just a requirement for the current implementation of shadow memory. If it does not support cross-context access one day, we can adjust our implementation for shadow memory. Also, it is hard to make GPU team commit to that requirement because in theory, we should not do cross-context access.

So you mean, this test is just an indicator for our current implementation. When it is failed, we may consider make a new implementation?

[AllanZyne]

I suggest move this test to "VirtualMem" folder, because if it is failed, SYCL runtime team will take responsible for it.

I don't think that is ideal because I think they don't care about cross context access as that may be a violation to the spec.

Since this is our requirement, we need to communicate with them to support this feature. But "VirutalMem" related issue is not our sanitizer team's responsibility. Anyway, we need them aware this requirement.

If this becomes a requirement, then the requirement should be made to L0/GPU team instead of SYCL team.

Yep, I updated my words, it's responsibility of owners of "VirutalMem" folder, not "AddressSaniitzer".

I still think we should keep this test in ASAN folder, because 1: this is not really a legitimate test for VirtualMem(cross-context), and 2: this would help us catch the problem sooner, rather than wait for the issue to be dispatched.

Got it. We need to know its status firstly.

[yingcong-wu]

Also, this is not a hard requirement for the whole DASAN, but just a requirement for the current implementation of shadow memory. If it does not support cross-context access one day, we can adjust our implementation for shadow memory. Also, it is hard to make GPU team commit to that requirement because in theory, we should not do cross-context access.

So you mean, this test is just an indicator for our current implementation. When it is failed, we may consider make a new implementation?

Yes! That is my intention here.

[yingcong-wu]

Hi @kbenzie , after retrigging the CI with an empty commit, the Jenkins/Precommit check has passed. At the time of writing this comment, there are no failed checks but 1 is still queued.

[yingcong-wu]

Hi @intel/llvm-gatekeepers , please help merge this PR, thanks.