Page MenuHomePhabricator

[DAGCombiner] Teach combineShiftToMULH to handle constant and const splat vector.
ClosedPublic

Authored by jacquesguan on Aug 16 2021, 6:20 AM.

Details

Summary

Fold (srl (mul (zext i32:$a to i64), i64:c), 32) -> (mulhu $a, $b) if c can truncate to i32 without loss.

Diff Detail

Event Timeline

jacquesguan created this revision.Aug 16 2021, 6:20 AM
jacquesguan requested review of this revision.Aug 16 2021, 6:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 6:20 AM

Update mulhu case for (mulhu x, (1 << c)) -> x >> (bitwidth - c).

I'm adding some non-RVV reviewers in this field; that may help move this patch along.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8525

I think the way HasConstant is interacting with checks below this could do with explanation (in the code). It's not clear to me why and how it interacts with logic later in this function.

8583–8588

I might be narrowly-focussed here, but I'm not sure why we need a ternary here. Can't we capture RightOp.getOperand(0) in a variable and ensure it's correct in the the if (HasConstant) block? Would that simplify any of the other logic?

RKSimon added inline comments.Sep 22 2021, 2:32 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8536

Why not reuse LeftConstant/RightConstant?

craig.topper added inline comments.Sep 22 2021, 9:35 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8519

Do we need to worry about Constant LHS? Won't the mul be canonicalized and then the shift will be revisited?

8535

Isn't ActiveBits the wrong thing to check for SIGN_EXTEND?

llvm/test/CodeGen/RISCV/rvv/vmulh-sdnode-rv32.ll
69

Was this test case supposed to match to mulh?

Refactor code and support negative signed constant.

jacquesguan added inline comments.Sep 23 2021, 4:49 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8519

Fixed, thanks a lot.

8535

Done, add check for negative signed constant, and postive constant shares same logic as unsigned one.

8536

Done, thanks a lot.

llvm/test/CodeGen/RISCV/rvv/vmulh-sdnode-rv32.ll
69

Done, thanks a lot.

Refactor code.

jacquesguan added inline comments.Sep 23 2021, 5:24 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8583–8588

Done, thank you.

Can the tests be pre-commited to trunk so we can see the effect of the patch on codegen?

craig.topper added inline comments.Sep 27 2021, 12:28 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8553

Fold this into the if on line 8530 to limit the scope of Constant

8558

Why can't we use getMinSignedBits() for signed and getActiveBits() for unsigned.

Refactor code.

Can the tests be pre-commited to trunk so we can see the effect of the patch on codegen?

Done, I pre-commited these tests in D110675.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8553

Done, thanks a lot.

8558

Done, thanks a lot.

What about rv64?

craig.topper added inline comments.Oct 24 2021, 6:44 PM
llvm/test/CodeGen/RISCV/rvv/vmulh-sdnode-rv32.ll
1

I have renamed this file to remove "-rv32" and added an rv64 command line. Do the same to the new file please.

Rebase and add RV64 tests.

jacquesguan added inline comments.Oct 28 2021, 8:23 PM
llvm/test/CodeGen/RISCV/rvv/vmulh-sdnode-rv32.ll
1

Done, thanks a lot.

Format code.

craig.topper added inline comments.Sun, Oct 31, 10:16 AM
llvm/test/CodeGen/RISCV/rvv/vmulhu-sdnode.ll
3 ↗(On Diff #383244)

One of these RUN lines should be -mtriple=riscv64

craig.topper added inline comments.Sun, Oct 31, 10:17 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8546–8547

Can you re-write this to

if (!IsSignExt && !ZeroExt)

jacquesguan added inline comments.Sun, Oct 31, 7:10 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8546–8547

Done

llvm/test/CodeGen/RISCV/rvv/vmulhu-sdnode.ll
3 ↗(On Diff #383244)

Done.

frasercrmck accepted this revision.Mon, Nov 1, 3:03 AM

LGTM but either you need to land D110675 and rebase, or add D110675 as a parent revision. That way it's easier to see how this patch improves things.

This revision is now accepted and ready to land.Mon, Nov 1, 3:03 AM
RKSimon requested changes to this revision.Mon, Nov 1, 3:47 AM

Please can you rebase on top of D110675 - the whole point of the precommit is to show the codegen diff

This revision now requires changes to proceed.Mon, Nov 1, 3:47 AM

Rebase main.

LGTM but either you need to land D110675 and rebase, or add D110675 as a parent revision. That way it's easier to see how this patch improves things.

Done.

Please can you rebase on top of D110675 - the whole point of the precommit is to show the codegen diff

Done.

RKSimon accepted this revision.Tue, Nov 2, 2:29 AM

LGTM - cheers

This revision is now accepted and ready to land.Tue, Nov 2, 2:29 AM
This revision was landed with ongoing or failed builds.Tue, Nov 2, 5:05 AM
This revision was automatically updated to reflect the committed changes.