Page MenuHomePhabricator

[RISCV] Legalize select when Zbt extension available
Needs ReviewPublic

Authored by mundaym on Wed, Dec 23, 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.Wed, Dec 23, 8:54 AM
mundaym requested review of this revision.Wed, Dec 23, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Dec 23, 8:54 AM
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
800–816

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

mundaym added inline comments.Wed, Dec 23, 1:31 PM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
800–816

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.Wed, Dec 23, 6:20 PM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
800–816

I think my preference is towards fewer patterns.

lenary added a comment.Mon, Jan 4, 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
800–816

@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.Tue, Jan 5, 5:14 AM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
800–816

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.Mon, Jan 11, 10:55 AM
lenary accepted this revision.Tue, Jan 12, 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.Tue, Jan 12, 1:24 PM
This revision was automatically updated to reflect the committed changes.
lenary reopened this revision.Thu, Jan 14, 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.Thu, Jan 14, 8:47 AM
lenary resigned from this revision.Thu, Jan 14, 8:48 AM
lenary added 1 blocking reviewer(s): luismarques.
This revision now requires review to proceed.Thu, Jan 14, 8:48 AM
mundaym updated this revision to Diff 317426.Mon, Jan 18, 3:48 PM

Fix inverted patterns.