This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Propagate !noalias and !alias.scope metadata in AMDGPULowerKernelArguments for noalias arguments.
Needs ReviewPublic

Authored by abinavpp on Aug 19 2021, 3:48 AM.

Details

Summary

This change requires https://reviews.llvm.org/D108361.

This patch handles noalias function arguments of amdgpu_kernel during
llvm.amdgcn.kernarg.segment.ptr() emission by converting them to the equivalent
metadata representation using !noalias and !alias.scope.

Diff Detail

Event Timeline

abinavpp created this revision.Aug 19 2021, 3:48 AM
abinavpp requested review of this revision.Aug 19 2021, 3:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 3:48 AM
llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
1280

What is the purpose of this test ? The kernel input pointers are 'stored to undef', as such those stores see the available noalias scopes, but do not take part in it. Shouldn't the test store something to the pointers instead ?

abinavpp added inline comments.Aug 19 2021, 5:15 AM
llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
1280

I'm not sure about the stores to undefs in AMDGPU/lower-kernargs.ll. This test was added in the initial commit for AMDGPULowerKernelArguments. I assume we're just checking the generated IR for the stores and not worrying about the destination. @arsenm is that right?

Same purpose as D105721?

arsenm added inline comments.Aug 23 2021, 1:06 PM
llvm/test/CodeGen/AMDGPU/lower-kernargs.ll
1280

It's just a dummy use to show the handling of the value

Same purpose as D105721?

Oh! I didn't notice that. @hliao , how should we move forward? Should we abandon this and https://reviews.llvm.org/D108361, or keep https://reviews.llvm.org/D108361 and use it for https://reviews.llvm.org/D105721?

Same purpose as D105721?

Oh! I didn't notice that. @hliao , how should we move forward? Should we abandon this and https://reviews.llvm.org/D108361, or keep https://reviews.llvm.org/D108361 and use it for https://reviews.llvm.org/D105721?

Ping. Was there any discussion of which way is better?

Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 6:22 AM
Herald added a subscriber: hsmhsm. · View Herald Transcript
abinavpp updated this revision to Diff 422118.Apr 11 2022, 9:31 PM

Rebased; Updated tests

Same purpose as D105721?

Oh! I didn't notice that. @hliao , how should we move forward? Should we abandon this and https://reviews.llvm.org/D108361, or keep https://reviews.llvm.org/D108361 and use it for https://reviews.llvm.org/D105721?

Ping. Was there any discussion of which way is better?

No, sorry. @hliao what do you think?

I'll check this patch with PSDB.

Same purpose as D105721?

Oh! I didn't notice that. @hliao , how should we move forward? Should we abandon this and https://reviews.llvm.org/D108361, or keep https://reviews.llvm.org/D108361 and use it for https://reviews.llvm.org/D105721?

Ping. Was there any discussion of which way is better?

No, sorry. @hliao what do you think?

I'll check this patch with PSDB.

PSDB's result of this patch with D108361 is a conditional go.