This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add a new select combine for when the condition is a setcc that will be inverted
Needs ReviewPublic

Authored by craig.topper on Jul 14 2023, 5:35 AM.

Details

Reviewers
reames
mgudim
asb
Summary

This is primarily motivated by improving codegen for zicond (fixing the code quality regressions knowingly introduced in D155083), but the combine is enabled regardless of the presence of zicond as it appears to be either neutral or provide minor benefits even without zicond.

Because we lower select early (due to it not being a legal operation), we miss out on some of the select combines that would kick in after other operations have been legalised - in this case if the condval is a setcc that will later be inverted when being legalised, the xori instruction can be removed just by flipping the TrueVal and FalseVal. This combine just performs that transformation.

Now that D155328 was committed, the primary benefit is for RV32.

Diff Detail

Event Timeline

asb created this revision.Jul 14 2023, 5:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 5:35 AM
asb requested review of this revision.Jul 14 2023, 5:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 5:35 AM
asb edited the summary of this revision. (Show Details)Jul 14 2023, 5:36 AM
mgudim added inline comments.Jul 14 2023, 10:32 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12300

what about floating-point?

12301

is it better to use TLI.getCondCodeAction(...) == Expand?

mgudim added inline comments.Jul 14 2023, 11:12 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12300

nvm, I understand why its limited to integers only.

asb updated this revision to Diff 542519.Jul 20 2023, 7:42 AM
asb edited the summary of this revision. (Show Details)

Rebase now D155328 landed.

I happened to see a case which looked a lot like this on RV64 where the user was a branch (probably not a lowered select, but not 100% sure). Could we extend this to a SETCC feeding a BRCOND as well? Doing this during DAG (vs only lowering) might expose more CSE opportunities.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
12849

This seems to have a decent amount of overlap with translateSetCCForBranch which we do during lowering of both branch and select. Could we maybe reuse that set of transforms here?

craig.topper commandeered this revision.Aug 16 2023, 1:51 PM
craig.topper edited reviewers, added: asb; removed: craig.topper.

Commandeering so I can rebase with latest test diffs

Looks like we're only seeing any benefit on forced-atomics.ll now? @reames can you share a reproducer for the BR_CC issue?

Looks like we're only seeing any benefit on forced-atomics.ll now? @reames can you share a reproducer for the BR_CC issue?

Will do. Will probably take me a couple days; I had been working only from a binary, and need to go figure out the build system for said benchmark.

This comment was removed by craig.topper.

Ignore the bad comment I just deleted. I had the wrong window.