This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Select s_cselect
ClosedPublic

Authored by piotr on Jun 16 2020, 4:08 AM.

Details

Summary

Add patterns to select s_cselect in the isel.

Handle more cases of implicit SCC accesses in si-fix-sgpr-copies
to allow new patterns to work.

Diff Detail

Event Timeline

piotr created this revision.Jun 16 2020, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 4:08 AM
foad added a subscriber: foad.Jun 16 2020, 6:34 AM
arsenm added inline comments.Jun 16 2020, 7:06 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
609

This should not be changed. We now report zero extended booleans since a few months ago

642

Should be 1

5577–5579

I'm not super comfortable with a search for the def like this, but there probably isn't a better option. I also care less about this code now that GlobalISel does this correctly.

Can you check if SCCSource is undef? I think that's the case this might crash on since there will be no def to find. The other cases should be caught by the verifier

6243–6244

Probably need to note this is really the expected SCC users to handle, since it's more restricted than checking if there's an scc use

6264

Braces

llvm/lib/Target/AMDGPU/SOPInstructions.td
470

I have a patch for GlobalISel to move select matching into patterns, which will be incompatible with this since the type of SCC is switched to i32. I guess this doesn't matter, since we're manually selecting it there already.

piotr updated this revision to Diff 271726.Jun 18 2020, 8:14 AM

Addressed review comments.

piotr marked 6 inline comments as done.Jun 18 2020, 8:20 AM
piotr added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
609

Thanks, I was confused by this, mainly because this global change here did not cause any issues in my tests.

I rewrote this part and moved the relevant functionality to lowerSelect.

5577–5579

Added the check.

Yes, this patch is not as neat as I would like due to the extra handling of SCC.

llvm/lib/Target/AMDGPU/SOPInstructions.td
470

Sounds good.

Btw, some s_cselect's are already generated for the global-isel at this point. It was just the old isel that did not have any support for that.

Can you also add a MIR test that at least covers the undef case?

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
5578

Maybe add a search range limit?

piotr updated this revision to Diff 271854.Jun 18 2020, 2:54 PM

Added MIR test to test undef SCC, added minor tweaks in lowerSelect and rebased.

piotr marked an inline comment as done.Jun 18 2020, 3:01 PM
piotr added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
5578

Not sure - the code relies on finding SCC def, and by construction (.td pattern) the def is very close to S_CSELECT's implicit use anyway.

arsenm accepted this revision.Jun 18 2020, 4:58 PM
This revision is now accepted and ready to land.Jun 18 2020, 4:58 PM
This revision was automatically updated to reflect the committed changes.