This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add patterns for RV64I SLLW/SRLW/SRAW instructions
ClosedPublic

Authored by asb on Jan 3 2019, 3:46 AM.

Details

Summary

This restores support for selecting the SLLW/SRLW/SRAW instructions, which was removed in rL348067 as the previous patterns made some unsafe assumptions. Also see the related llvm-dev discussion.

Ultimately I didn't introduce a custom SelectionDAG node, but instead added a DAG combine that inserts an AssertZext i5 on the shift amount for an i32 variable-length shift and also added an ANY_EXTEND DAG-combine which will instead produce a SIGN_EXTEND for an i32 variable-length shift, increasing the opportunity to safely select SLLW/SRLW/SRAW.

There are obviously different ways of addressing this (a number discussed in the llvm-dev thread), so I'd welcome further feedback and comments.

Note that there are now some cases in test/CodeGen/RISCV/rv64i-exhaustive-w-insts.ll where sraw/srlw/sllw is selected even though sra/srl/sll could be used without any extra instructions. Given both are semantically equivalent, there doesn't seem a good reason to prefer one vs the other. Given that would require more logic to still select sra/srl/sll in those cases, I've left it preferring the *w variants.

Diff Detail

Event Timeline

asb created this revision.Jan 3 2019, 3:46 AM
asb updated this revision to Diff 180107.Jan 3 2019, 11:16 AM

Updated to improve pattern definitions. The first patterns were over-constrained, as they would only match shifts where the shift amount operand matched the zexti32 pattern. I introduce the shiftwamt PatFrags so it will match a zexti32 shift amount (and hence avoid generating the unnecessary zero-extend), or a shift amount that isn't zero-extended.

efriedma added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
543

Is it worth detecting an existing AssertZext with a value type wider than 5 bits?

lib/Target/RISCV/RISCVInstrInfo.td
665

<= 5?

671

The literal "0xffffffff" could probably be generalized a bit.

asb updated this revision to Diff 181045.Jan 10 2019, 6:33 AM
asb marked 3 inline comments as done.

Updated to address review comments from Eli (thanks!).

asb updated this revision to Diff 181046.Jan 10 2019, 6:34 AM

Previous patch was the pre-clang-format version. This update fixes that!

asb edited reviewers, added: efriedma; removed: eli.friedman.Jan 11 2019, 2:22 PM
efriedma accepted this revision.Jan 11 2019, 3:41 PM
This revision is now accepted and ready to land.Jan 11 2019, 3:41 PM
This revision was automatically updated to reflect the committed changes.
rogfer01 added inline comments.Jan 16 2019, 6:24 AM
llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp
554 ↗(On Diff #181425)

I've raised https://bugs.llvm.org/show_bug.cgi?id=40333 because this interacts badly with TargetLowering::SimplifyDemandedBits. This newly created SIGN_EXTEND is turned back into an ANY_EXTEND when it is used by an (AND x, 255)

Kind regards,

asb marked an inline comment as done.Jan 16 2019, 6:39 AM
asb added inline comments.
llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp
554 ↗(On Diff #181425)

Thanks for the report, I'll take a look.

asb marked an inline comment as done.Jan 22 2019, 10:48 PM
asb added inline comments.
llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp
554 ↗(On Diff #181425)

Worked around in rL351806 and D57085 posted to implement an alternative lowering / legalisation strategy.