This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix SGPR checks in S_MOV_B64_IMM_PSEUDO generation.
ClosedPublic

Authored by abinavpp on Nov 2 2021, 4:30 AM.

Details

Summary

The function to generate S_MOV_B64_IMM_PSEUDO was recently modified to
optimize AGPR to AGPR copy but it missed checking for the SGPR
clobbering for the S_MOV_B64_IMM_PSEUDO generation.

Diff Detail

Event Timeline

abinavpp created this revision.Nov 2 2021, 4:30 AM
abinavpp requested review of this revision.Nov 2 2021, 4:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 4:30 AM

This fixes the codegen of test_rocrand_kernel_xorwow.cpp of rocRAND (tracked by SWDEV-306338).

arsenm added inline comments.Nov 2 2021, 7:07 AM
llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp
96–97

Can you just change the range loop to preincrement the iterator?

102–116

Checking for specific opcodes with specific subreg indices is definitely not the right way to check for register interference.

vangthao added inline comments.Nov 2 2021, 10:20 AM
llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp
102

Why do we check for COPY here? If Reg is not AGPR then we will not touch any COPY instructions.

119

Is there a reason why we need a new loop to check this?

This should do the same as the removed lines of code below:

if (Def0)
  return false;
if (Def1)
  return false;

As in if there are more than one def of a subreg then bail.

rampitec added inline comments.Nov 2 2021, 10:37 AM
llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp
150

If you do a separate loop I do not understand why this handling is still in the second loop.

abinavpp updated this revision to Diff 384297.Nov 2 2021, 6:28 PM

We don't need the new loop. Sorry for the noise. We just need to make sure that
we don't do the S_MOV_B64_IMM_PSEUDO generation when we have an SGPR COPY like
how D104874 did.

abinavpp retitled this revision from [AMDGPU] Fix subreg checks in S_MOV_B64_IMM_PSEUDO generation. to [AMDGPU] Fix SGPR checks in S_MOV_B64_IMM_PSEUDO generation..Nov 2 2021, 6:34 PM
abinavpp edited the summary of this revision. (Show Details)
vangthao accepted this revision.Nov 2 2021, 7:47 PM

LGTM.

This revision is now accepted and ready to land.Nov 2 2021, 7:47 PM