This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Legalize select when Zbt extension available
ClosedPublic

Authored by mundaym on Dec 23 2020, 8:54 AM.

Details

Summary

The custom expansion of select operations in the RISC-V backend
interferes with the matching of cmov instructions. Legalizing
select when the Zbt extension is available solves that problem.

Diff Detail

Event Timeline

mundaym created this revision.Dec 23 2020, 8:54 AM
mundaym requested review of this revision.Dec 23 2020, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2020, 8:54 AM
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
780

Is there a difference between doing it this way versus just adding more riscv_selectcc patterns for other condition codes?

mundaym added inline comments.Dec 23 2020, 1:31 PM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
780

No I think either would work. Using riscv_selectcc would have the advantage that we'd only encounter normalised condition codes so we'd need fewer patterns. Do you have a preference?

craig.topper added inline comments.Dec 23 2020, 6:20 PM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
780

I think my preference is towards fewer patterns.

lenary added a comment.Jan 4 2021, 9:17 AM

The code generation improvements here are looking good, let's hope they stay with a set of riscv_selectcc patterns.

llvm/lib/Target/RISCV/RISCVInstrInfoB.td
780

@mundaym and I discussed this today, and fewer patterns makes sense, though there's a slight hiccup with using CondCode directly in the riscv_selectcc pattern that he's looking into.

mundaym added inline comments.Jan 5 2021, 5:14 AM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
780

After further investigation it looks like using riscv_selectcc will end up requiring roughly the same number of patterns because we need to lower each legal comparison operation (SETEQ, SETGE, etc.) individually whereas with select we only need to do it for comparison operations that benefit from optimization.

I think we should therefore stick with the original approach I took of legalizing select with some additional optimization patterns. This approach also bypasses the issue @lenary mentioned where the comparison operation (e.g. SETEQ) currently has to be encoded as an immediate in riscv_selectcc (e.g. XLenVT 17) which is a bit fragile, especially if more patterns are added.

This revision is now accepted and ready to land.Jan 11 2021, 10:55 AM
lenary accepted this revision.Jan 12 2021, 1:13 PM

LGTM!

I'm going to look into removing more of the xori … 1 which are used by the selects in some of these cases.

This revision was landed with ongoing or failed builds.Jan 12 2021, 1:24 PM
This revision was automatically updated to reflect the committed changes.
lenary reopened this revision.Jan 14 2021, 8:47 AM

We've found some issues in testing, so for the moment I've reverted this patch in rG7c9c2a2ea5e3760d7310309c96c9a4ce41fa4d9b.

This revision is now accepted and ready to land.Jan 14 2021, 8:47 AM
lenary resigned from this revision.Jan 14 2021, 8:48 AM
lenary added 1 blocking reviewer(s): luismarques.
This revision now requires review to proceed.Jan 14 2021, 8:48 AM
mundaym updated this revision to Diff 317426.Jan 18 2021, 3:48 PM

Fix inverted patterns.

luismarques accepted this revision.Jan 21 2021, 10:15 AM

LGTM. The issues we detected are now solved.

This revision is now accepted and ready to land.Jan 21 2021, 10:15 AM

@mundaym do you have commit access yet? I think Sam had been committing your previous patches?

@craig.topper No, I don't have commit access yet. (Thanks for reviewing this by the way!)