Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
80 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
80 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-static-initializer.S
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-static-initializer.S
90 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-tls.S
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-tls.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-tls.S

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
8524

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.

8574–8575

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
8535

Why not reuse LeftConstant/RightConstant?

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

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

8534

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
8518

Fixed, thanks a lot.

8534

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

8535

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
8574–8575

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
8528

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

8533

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
8528

Done, thanks a lot.

8533

Done, thanks a lot.

What about rv64?

craig.topper added inline comments.Sun, Oct 24, 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.