This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Don't form MULW for (sext_inreg (mul X, Y), i32)) if the mul has another use.
AbandonedPublic

Authored by craig.topper on Mar 20 2021, 6:11 PM.

Details

Summary

If this pattern doesn't fully remove the mul we'll end up with
a MUL and MULW with the same inputs.

On the assumption that multipliers are a limited resource and have
higher latency than an add, it is likely better to use a single MUL
and a sext.w instruction.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 20 2021, 6:11 PM
craig.topper requested review of this revision.Mar 20 2021, 6:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2021, 6:11 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Mar 20 2021, 6:23 PM
llvm/test/CodeGen/RISCV/xaluo.ll
703

This is an interesting case. The mul is used by the sext.w and the sw. Both only consume the lower 32 bits. So it would be better to use MULW for the sext_inreg+mul and pass that result to the sw.

Pretty sure anything we do to try to make that happen in DAG combine or lowering will be defeated by SimplifyDemandedBits. Unless we add a RISCVISD::MULW node to hide it.

We could probably catch this specific case with a PreprocessISelDAG peephole that would like for a sext_inreg+mul. We could examine all other users of the mul and see if they only need the lower 32 bits. We might not be able to catch many cases without recursively checking the users of the users though. Ideally we'd be able to see that the user was a ADDW/SUBW but that requires know the use is an add that is itself used by a sext_inreg.

1050–1052

This demonstrates that we now get full benefit of the (mul (and X, 0xffffffff), (and Y, 0xffffffff)) optimization.

frasercrmck added inline comments.Mar 24 2021, 2:48 AM
llvm/lib/Target/RISCV/RISCVInstrInfoM.td
74

Super nitty, but I see su and my mind doesn't jump to "single use". I think more "signed/unsigned". Maybe something like mul_oneuse?

Rename mul_su->mul_oneuse

I made a dedicated fix for the motivating issue with 5692fc38e0d17abc55a4a84da98f021a1d53d76d so I'm not as concerned with pushing this patch now.

asb added a comment.Mar 31 2021, 7:10 AM

I made a dedicated fix for the motivating issue with 5692fc38e0d17abc55a4a84da98f021a1d53d76d so I'm not as concerned with pushing this patch now.

Ack. The approach of adding a one-use check seems sensible, so if it does make sense to respin and push this further, obviously just ping here.

craig.topper planned changes to this revision.Apr 1 2021, 10:19 AM
craig.topper abandoned this revision.Aug 13 2021, 9:22 AM