[SYCL] Switch SPIR-V offload target to opaque pointers

[bader]

Although there are a few tests failing due to this change, we need to go with this change to avoid future regressions and unblock changes removing typed pointers support. The regressions are supposed to be fixed by follow-up patches.

[jsji]

So any use of the SYCL vector math functions in a ESIMD kernel seems to cause the problem.

Yes, ext_math is the test in question, and the issue was in the GPU RT. It should be fixed with intel/intel-graphics-compiler@c083dec which made it into igc 1.0.14342.

@sarnex Is this problem happen only with the OLD driver? updating the igc will fix these? Or they will fail even with updated igc?

[sarnex]

@jsji A driver with IGC 1.0.14342 or later will not have the problem and the test will pass.

[jsji]

@jsji A driver with IGC 1.0.14342 or later will not have the problem and the test will pass.

Great to hear that! Then let us try to upgrade the IGC driver in dependencies and CI.

[sarnex]

@jsji Sounds good, let me know if you hit any problems with this test on a new driver and I will investigate.

[jsji]

@bader @aelovikov-intel @intel/dpcpp-devops-reviewers Can we try to update the IGC version?

@v-klochkov If we can't update the IGC version due to some other reason for now, can we XFAIL the test as @sarnex confirmed that this is just due to old driver. We will eventually update the IGC version anyhow.

[sarnex]

@jsji The problem is not really about this test in particular, the problem is basic ESIMD functionality will break for people not using the latest drivers. Even in internal testing we don't seem to be using a driver that has the fix.

[bader]

@jsji The problem is not really about this test in particular, the problem is basic ESIMD functionality will break for people not using the latest drivers. Even in internal testing we don't seem to be using a driver that has the fix.

I have concern that only one test has failed, which seems to test something different. Don't we have ESIMD tests checking "basic functionality"? I would recommend carefully reviewing ESIMD tests as from my POV the ROI of current tests is low.

[bader]

@bader @aelovikov-intel @intel/dpcpp-devops-reviewers Can we try to update the IGC version?

I have no objections to check, but we need to test if using custom IGC version doesn't break other tests. We use the version specified by the GPU driver - https://github.com/intel/compute-runtime/releases, which is tested with the rest of the driver components.
@aelovikov-intel, could you run as much tests as possible with the IGC 1.0.14342 or later before the switch? I used to use regular pre-commit, but AFAIK, it doesn't work for testing changes in CI anymore.

[sarnex]

@bader It seems we only have one test that uses the vector sycl math functions and that test makes use of them extensively. By basic functionality, I meant that it is extremely easy to hit this problem. If they use the functions at all, even in trivial cases, they will hit the problem. I don't actually know how often these functions are used by customers, maybe @v-klochkov has more customer insight.

[jsji]

Don't we have ESIMD tests checking "basic functionality"? I would recommend carefully reviewing ESIMD tests as from my POV the ROI of current tests is low.

Yes, agree! But I think this shouldn't be a blocker for opaque pointer enablement.

[jsji]

@jsji The problem is not really about this test in particular, the problem is basic ESIMD functionality will break for people not using the latest drivers. Even in internal testing we don't seem to be using a driver that has the fix.

In theory, when we release the changes , we can require NEW drivers. But yes, we will need to test and update the new drivers in all our relevant tests including internal tests. And also involve the release managers.

[aelovikov-intel]

@aelovikov-intel, could you run as much tests as possible with the IGC 1.0.14342 or later before the switch? I used to use regular pre-commit, but AFAIK, it doesn't work for testing changes in CI anymore.

Driver update isn't considered to be CI changes. You should be able to test it.

[sommerlukas]

Reviewed the kernel fusion test, makes sense to deactivate for now.

We'll investigate the issue and address in a separate PR.

[bader]

We'll investigate the issue and address in a separate PR.

Great. Thank you!

[bader]

@sarnex, the version is use in CI is the latest publicly available IGC version. To update the IGC version, we need to wait for a public release.

I don't actually know how often these functions are used by customers, maybe @v-klochkov has more customer insight.

@v-klochkov, @turinevgeny, please, approve if it's okay to have sycl/test-e2e/ESIMD/ext_math.cpp disabled while we are waiting for a new GPU driver.

[bader]

@intel/dpcpp-tools-reviewers, @intel/llvm-reviewers-runtime, ping.

[asudarsa]

Hi @bader

Overall looks ok to turn this on. I do not see any major issues, though it might help to resolve the failures soon as this will block a few folks. If you think it's a good idea, please open an issue for resolving these fails.

Thanks

[bader]

I do not see any major issues, though it might help to resolve the failures soon as this will block a few folks.

Who might be blocked by this?

If you think it's a good idea, please open an issue for resolving these fails.

I already did - #10682.

[asudarsa]

I do not see any major issues, though it might help to resolve the failures soon as this will block a few folks.

Who might be blocked by this?

I think bfloat issue might block OneDNN etc. But it was more of a blanket statement. Not a blocker here.

If you think it's a good idea, please open an issue for resolving these fails.

I already did - #10682.

Just spotted it in the earlier comments. Sorry for missing it. I will try to see if I can help there. Thanks again

[bader]

@againull, @intel/llvm-reviewers-runtime, ping.

[againull]

No concerns from SYCL RT point of view.