Page MenuHomePhabricator

[TargetLowering][RISCV] Introduce getExtendForShiftAmount and implement for RISC-V
AbandonedPublic

Authored by asb on Oct 8 2018, 1:19 AM.

Details

Summary

As of rL125457, the shift amount operand is always zero-extended when being promoted. This is conservatively safe, but on some targets (e.g. RISC-V) this leads to unnecessary instructions as the native shift operations ignore all but the lower 5/6 bits.

This patch introduces the getExtendForShiftAmount hook which is called from SelectionDAGBuilder::visitShift and implements that hook for RISC-V. The benefit can be seen in the promoted 8 and 16-bit shifts in the test/CodeGen/RISCV/alu{8,16}.ll test cases.

Diff Detail

Event Timeline

asb created this revision.Oct 8 2018, 1:19 AM
asb edited the summary of this revision. (Show Details)Oct 8 2018, 1:22 AM
asb updated this revision to Diff 168621.Oct 8 2018, 1:37 AM

Update diff to remove the TODO in alu8.ll and alu16.ll, as this patch addresses that problem.

efriedma requested changes to this revision.Oct 8 2018, 12:16 PM
efriedma added a subscriber: efriedma.

You can't ANY_EXTEND here; that will confuse DAGCombine, which will, for example, fold shifts where the shift amount is too large to undef. Other targets normally deal with this at isel time; see, for example, MaskedShiftAmountPats in lib/Target/X86/X86InstrCompiler.td .

This revision now requires changes to proceed.Oct 8 2018, 12:16 PM
asb added a comment.Oct 8 2018, 6:25 PM

Thanks for the review, I was hoping there was an opportunity here to avoid the need for backend-specific peepholes. I'll handle in the backend instead. For my own edification, what do you mean by the shift amount being too large to undef?

I guess that sentence was weird. I meant, if the shift amount is too large, DAGCombine will fold the shift to undef.

I was hoping there was an opportunity here to avoid the need for backend-specific peepholes

It's possible there's some room for improvement here, to add some target-independent infrastructure for shift instructions that mask the shift amount. But nothing exists at the moment, and I'm not sure what the best approach would be.

asb abandoned this revision.Oct 12 2018, 3:38 PM

Thanks for the feedback @efriedma, I've posted a patch that tackles this at instruction selection time: D53224.