This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Promote recursive loads from kernel argument to constant
ClosedPublic

Authored by rampitec on Feb 15 2022, 12:51 PM.

Details

Summary

Not clobbered pointer load chains are promoted to global now. That
is possible to promote these loads itself into constant address
space. Loaded pointers still need to point to global because we
need to be able to store into that pointer and because an actual
load from it may occur after a clobber.

Diff Detail

Event Timeline

rampitec created this revision.Feb 15 2022, 12:51 PM
rampitec requested review of this revision.Feb 15 2022, 12:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2022, 12:51 PM
Herald added a subscriber: wdng. · View Herald Transcript
rampitec updated this revision to Diff 409060.Feb 15 2022, 2:31 PM

Updated CUDA test.

vpykhtin added inline comments.Feb 16 2022, 7:24 AM
llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
133

Nit: you can leave only the same check after the loop below

138

Nit: you can shorten the loop unsing while (auto *P = dyn_cast<AddrSpaceCastInst>(OrigPtr)) ...

rampitec added inline comments.Feb 16 2022, 8:07 AM
llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
133

Not really. First check is if the original pointer is already a constant address space, the one below is it was cast from constant. In either case we need to bail.

138

I need both Ptr and OrigPtr after the loop.

rampitec planned changes to this revision.Feb 16 2022, 8:36 AM

It promotes atomics, I need to fix it.

arsenm added inline comments.Feb 16 2022, 8:37 AM
llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
138

stripPointerCasts?

rampitec added inline comments.Feb 16 2022, 8:43 AM
llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
138

That's too much, I only need to strip address space casts. stripPointerCasts may change type.

arsenm added inline comments.Feb 16 2022, 8:55 AM
llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
138

addrspacecast may also change the element type, it's just not canonical

rampitec updated this revision to Diff 409293.Feb 16 2022, 9:22 AM

Fixed atomic loads promotion.

rampitec updated this revision to Diff 409346.Feb 16 2022, 11:31 AM
rampitec marked an inline comment as done.

Check pointee type in case addrspacecast changes it.

llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
138

Good point. Checking pointee type now.

PSDB has passed.

vpykhtin accepted this revision.Feb 17 2022, 12:24 AM

LGTM.

llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
155

I'm not sure if this would be easier to understand if you use Ptr->getType() here as you checked its the same as OrigPtr->getType()

This revision is now accepted and ready to land.Feb 17 2022, 12:24 AM
rampitec added inline comments.Feb 17 2022, 12:38 AM
llvm/lib/Target/AMDGPU/AMDGPUPromoteKernelArguments.cpp
155

Except for the address space it shall be exactly the same as the original pointer all the way of the cast chain. This is the whole point of the checks above.

This is somewhat awkward to have a cast chain, but the alternative is to replicate a whole infer-address-space logic.

This revision was landed with ongoing or failed builds.Feb 17 2022, 11:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2022, 11:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript