This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Support v_mov_b64 in dpp combine
ClosedPublic

Authored by rampitec on Mar 10 2022, 1:53 PM.

Diff Detail

Event Timeline

rampitec created this revision.Mar 10 2022, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 1:53 PM
rampitec requested review of this revision.Mar 10 2022, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 1:53 PM
Herald added a subscriber: wdng. · View Herald Transcript
foad accepted this revision.Mar 11 2022, 2:40 AM

Looks OK.

As an alternative, maybe we could remove V_MOV_B64_DPP_PSEUDO, use the pseudo V_MOV_B64_dpp on all targets, and expand it post-ra only on targets that don't support it?

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
169

I wonder why we don't include V_MOV_B32_e64? Maybe we never use it for an immediate copy?

This revision is now accepted and ready to land.Mar 11 2022, 2:40 AM

Looks OK.

As an alternative, maybe we could remove V_MOV_B64_DPP_PSEUDO, use the pseudo V_MOV_B64_dpp on all targets, and expand it post-ra only on targets that don't support it?

This maybe a good idea at this point. As a separate change of course.

llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
169

Probably... Or perhaps it was just forgotten.

Joe_Nash accepted this revision.Mar 11 2022, 11:39 AM
Joe_Nash added a subscriber: Joe_Nash.
Joe_Nash added inline comments.
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
169

The only reason I can think of to use V_MOV_B32_e64 over V_MOV_B32_e32 is to use operand modifiers, and our backend doesn't even support those for V_MOV_B32_e64 :) But I agree it should be present here, for completeness.

This revision was landed with ongoing or failed builds.Mar 11 2022, 11:58 AM
This revision was automatically updated to reflect the committed changes.