The ray_origin, ray_dir and ray_inv_dir arguments should all be vec3 to
match how the hardware instruction works.
Also update the API of the corresponding OpenCL builtins.
Paths
| Differential D114957
[AMDGPU] Change llvm.amdgcn.image.bvh.intersect.ray to take vec3 args AbandonedPublic Authored by foad on Dec 2 2021, 6:39 AM.
Details
Diff Detail
Event TimelineHerald added subscribers: kerbowa, hiraditya, t-tye and 5 others. · View Herald TranscriptDec 2 2021, 6:39 AM Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 2 2021, 6:39 AM foad added parent revisions: D114956: [IR,TableGen] Add support for vec3 intrinsic arguments, D114955: [AMDGPU] Generate checks for llvm.amdgcn.image.bvh.intersect.ray.Dec 2 2021, 6:39 AM Comment ActionsThis is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK? Comment Actions
This breaks users' code. If we have to do this, at least let clang emit a pre-defined macro e.g. __amdgcn_bvh_use_vec3__=1 so that users can make their code work before and after the change. Comment Actions
I do not think it's worth introducing a macro for this. Are there actually C users of these builtins? Comment Actions
Yes we have users who use these clang builtins. We have received quite a few complaints about making breaking API changes without a way to detect them in the program. Comment Actions
But builtins are not part of the documented API and we have advised developers using them that they are subject to change. Comment Actions
I don't know anything about OpenCL macros. Is it good enough to put this in AMDGPUTargetInfo::getTargetDefines: if (Opts.OpenCL) Builder.defineMacro("__amdgcn_bvh_use_vec3__"); Does it need tests, documentation, etc? Comment Actions
But how long would that be carried? And then deprecated? Comment Actions
Then do you think the patch is OK as-is? Comment Actions
Let's discuss with the users and see whether the macro is needed or not. Comment Actions
Yes you can add it to AMDGPUTargetInfo::getTargetDefines but without if (Opts.OpenCL) since HIP uses it too. You can add a test to https://github.com/llvm/llvm-project/blob/b2a2c38349a18b89b04d342632d5ea02f86dfdd6/clang/test/Preprocessor/predefined-arch-macros.c#L3731 Comment Actions I think this macro is purely terrible and should not be added (and at least should be all caps?). If we can't just hard break users, I would rather just leave the builtin signatures broken Comment Actions
Rather than adding an ad-hoc named macro, could they just directly check the clang version? Comment Actions
For the previous and the next clang release, it can be determined by checking clang version. Problem is that ROCm release is not synch'ed with clang release. So for ROCm release that uses the current clang release, it could have this change or could not, which cannot be determined by checking clang version. Comment Actions
OK, how about D115032? Personally I have no opinion about what's best to do with the OpenCL builtins, but I would like to make progress with changing the intrinsics. So I have a slight preference for D115032 because it gives me a way forward without changing OpenCL behaviour. The OpenCL team can then decide whether or not to update the builtins at their leisure.
Revision Contents
Diff 391403 clang/include/clang/Basic/BuiltinsAMDGPU.def
clang/lib/Basic/Targets/AMDGPU.cpp
clang/test/CodeGenOpenCL/builtins-amdgcn-raytracing.cl
clang/test/Preprocessor/predefined-macros.c
llvm/include/llvm/IR/IntrinsicsAMDGPU.td
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.intersect_ray.ll
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.intersect_ray.ll
|