This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Change llvm.amdgcn.image.bvh.intersect.ray to take vec3 args
AbandonedPublic

Authored by foad on Dec 2 2021, 6:39 AM.

Details

Summary

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.

Diff Detail

Event Timeline

foad created this revision.Dec 2 2021, 6:39 AM
foad requested review of this revision.Dec 2 2021, 6:39 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 2 2021, 6:39 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

yaxunl added a comment.EditedDec 2 2021, 6:53 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

arsenm added a comment.Dec 2 2021, 6:55 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

I do not think it's worth introducing a macro for this. Are there actually C users of these builtins?

yaxunl added a comment.Dec 2 2021, 6:59 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

I do not think it's worth introducing a macro for this. Are there actually C users of these builtins?

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.

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

I do not think it's worth introducing a macro for this. Are there actually C users of these builtins?

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.

But builtins are not part of the documented API and we have advised developers using them that they are subject to change.

foad added a comment.Dec 2 2021, 7:16 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

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?

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

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?

But how long would that be carried? And then deprecated?

foad added a comment.Dec 2 2021, 7:30 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

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?

But how long would that be carried? And then deprecated?

Then do you think the patch is OK as-is?

yaxunl added a comment.Dec 2 2021, 7:48 AM

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

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?

But how long would that be carried? And then deprecated?

Then do you think the patch is OK as-is?

Let's discuss with the users and see whether the macro is needed or not.

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

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.

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?

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

b-sumner added a comment.EditedDec 2 2021, 10:11 AM

Can we agree to drop the macro in LLVM 15 and note that in a comment or elsewhere?

foad updated this revision to Diff 391403.Dec 2 2021, 11:13 AM

Define amdgcn_bvh_use_vec3.

LGTM from clang side. Thanks.

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

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

Rather than adding an ad-hoc named macro, could they just directly check the clang version?

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

Rather than adding an ad-hoc named macro, could they just directly check the clang version?

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.

foad added a comment.Dec 3 2021, 2:00 AM

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

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.

foad edited the summary of this revision. (Show Details)Dec 3 2021, 2:01 AM
foad abandoned this revision.Dec 7 2021, 9:31 AM

Abandoned in favour of D115032.