Page MenuHomePhabricator

[AMDGPU] Allow DPP combiner to work with REG_SEQUENCE
ClosedPublic

Authored by rampitec on Oct 10 2019, 1:25 PM.

Diff Detail

Event Timeline

rampitec created this revision.Oct 10 2019, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 1:25 PM
arsenm added inline comments.Oct 10 2019, 3:14 PM
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
420

Why is this a SetVector?

487–490

I think this won't work in the case where the operand itself has a subregister.
Can you add a test with something like

%0:vreg_64 = REG_SEQUENCE %vreg_64.sub0, sub1, %vreg_64.1, sub0

I think you can use composeSubRegIndices here

499

No else after continue

rampitec marked an inline comment as done.Oct 10 2019, 3:25 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
420

I will try to add if several times, for each subreg. Thus set. I guess it can be just SmallSet though.

rampitec marked 4 inline comments as done.Oct 10 2019, 3:52 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
487–490

It cannot directly happen because we are in SSA and def must be a result of mov_dpp, i.e. defining the whole register.
This can however happen if yet another reg_sequence is composed out of the first one. I have added checks and test.

rampitec updated this revision to Diff 224498.Oct 10 2019, 3:53 PM
rampitec marked an inline comment as done.

Addressed comments.

arsenm added inline comments.Oct 11 2019, 3:58 PM
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
535–539

This seems like a questionable way to preserve undefs. Can you avoid doing this by checking getVRegDef?

rampitec marked an inline comment as done.Oct 11 2019, 4:08 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
535–539

What's wrong with it? A register either has def or not. The other way would be to keep all reg_sequences along with the info about all subregs, if they were combined or not.

vpykhtin added inline comments.Oct 14 2019, 10:02 AM
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
356

How mov can define subreg in the SSA?

rampitec marked an inline comment as done.Oct 14 2019, 10:24 AM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
356

That is not necessarily a mov anymore, it can be a reg_sequence operand now. But I guess this is part of older patch, because all reg_sequences are processed inside this function now. I probably need to remove it.

rampitec updated this revision to Diff 224874.Oct 14 2019, 10:32 AM
rampitec marked an inline comment as done.

Removed dpp subreg handling, a subreg cannot be defined in SSA.

arsenm added inline comments.Oct 14 2019, 10:36 AM
llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp
535–539

What if the instruction were incorrectly erased? The undef flag would have been present and this would no longer catch it

rampitec updated this revision to Diff 224892.Oct 14 2019, 12:40 PM
rampitec marked 2 inline comments as done.

Full accounting for undefs.

This revision is now accepted and ready to land.Oct 15 2019, 6:27 AM
This revision was automatically updated to reflect the committed changes.