[SYCL] Fix bugs with recursion in SYCL kernel #3958
Conversation
For the checks we do in SemaSYCL via the callgraph, we don't want to diagnose when it is in a discarded statement. This adds the infrastructure for another bug, where we visit constexpr during this too.
This patch removes ConstexprDepthRAII which incorrectly maintained
constexpr context. Instead we don't traverse statements which are
forced constant expressions.
This patch fixes the following bugs -
1. constexpr int j = test_constexpr_context(recfn(1));
2. int k;
if constexpr (false)
k = recfn(1);
Here, recfn() is a recursive function. The usage 1. and 2. should not
diagnose in SYCL kernel since the function is called in constexpr
context.
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
elizabethandrews
requested review from
AaronBallman and
premanandrao
as code owners
|
According to SYCL spec "5.4. Language restrictions for device functions": Do we really need this change? The case looks explicitly restricted by the SYCL specification. |
It does, and if we do need this change, the SYCL spec needs to be fixed to clarify what it means. My understanding (from previous conversations) is that the spec is trying to disallow runtime recursion on the device, not trying to disallow compile-time recursion that happens in the implementation execution environment. Based on that, I think these changes are necessary for implementation consistency. |
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
elizabethandrews
dismissed stale reviews from AaronBallman and erichkeane
via
be2739c
|
Thanks for the review! |
|
@vladimirlaz @bader I think this patch is good to merge. Tests passed prior to last commit (which was a NFC renaming a variable). I tried restarting the failing tests but I'm not able to. |
I have restarted the tests. Will wait for completion... |
* upstream/sycl: (489 commits) [SYCL][NFC] Lower overhead of making plugin calls (intel#3982) [SYCL][NFC] Use default macro initialization where applicable (intel#3979) [SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension (intel#3864) [SYCL] Disable reassociate pass to reduce register pressure (intel#3615) [Driver][SYCL][FPGA] Restrict -O0 for FPGA with hardware (intel#3966) [SYCL][NFC] Fix warnings coming out of SYCL headers. (intel#3978) [SYCL] Fix bugs with recursion in SYCL kernel (intel#3958) [SYCL][LevelZero] Add support to detect host->device and device->host transfers for USM (intel#3975) [SYCL] Enable native FP atomics by default (intel#3869) [sycl-post-link] Avoid copying from nullptr (intel#3963) [SYCL-PTX] Add warp-reduce path in sub-group reduce (intel#3949) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#3946) Fix handling of complex constant expressions Handle OpSpecConstantOp with CompositeExtract and CompositeInsert Handle OpSpecConstantOp with VectorShuffle [FuncSpec] Don't specialise functions with NoDuplicate instructions. [mlir][linalg] Support low padding in subtensor(pad_tensor) lowering [gn build] Port 208332d [AMDGPU] Add Optimize VGPR LiveRange Pass. [mlir][Linalg] NFC - Drop unused variable definition. ...
This patch removes ConstexprDepthRAII which incorrectly maintained
constexpr context. Instead we don't traverse statements which are
forced constant expressions.
This patch fixes the following bugs -
if constexpr (false)
k = recfn(1);
Here, recfn() is a recursive function. The usage 1. should not
diagnose in SYCL kernel since the function is called in constexpr
context. Similarly 2. should not diagnose in SYCL kernel since the
recursive function call is in a discarded branch.
Includes commits in PR - #3714
Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com