[PATCH] D108129: [DAGCombiner] Teach combineShiftToMULH to handle constant and const splat vector.

[Jianjian Guan via Phabricator via llvm-commits]

jacquesguan added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8519
   SDValue RightOp = ShiftOperand.getOperand(1);
+  ConstantSDNode *LeftConstant = isConstOrConstSplat(LeftOp);
+  ConstantSDNode *RightConstant = isConstOrConstSplat(RightOp);
----------------
craig.topper wrote:
> Do we need to worry about Constant LHS? Won't the mul be canonicalized and then the shift will be revisited?
Fixed, thanks a lot.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8535
+        ExtOp.getOperand(0).getValueType().getScalarSizeInBits();
+    unsigned ActiveBits =
+        isConstOrConstSplat(ConstantOp)->getAPIntValue().getActiveBits();
----------------
craig.topper wrote:
> Isn't ActiveBits the wrong thing to check for SIGN_EXTEND?
Done, add check for negative signed constant, and postive constant shares same logic as unsigned one.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8536
+    unsigned ActiveBits =
+        isConstOrConstSplat(ConstantOp)->getAPIntValue().getActiveBits();
+    if (ActiveBits > NarrowVTBits)
----------------
RKSimon wrote:
> Why not reuse LeftConstant/RightConstant?
Done, thanks a lot.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vmulh-sdnode-rv32.ll:69
+; CHECK-NEXT:    addi a0, zero, 32
+; CHECK-NEXT:    vsrl.vx v25, v25, a0
+; CHECK-NEXT:    vsetvli zero, zero, e32, mf2, ta, mu
----------------
craig.topper wrote:
> Was this test case supposed to match to mulh?
Done, thanks a lot.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108129/new/

https://reviews.llvm.org/D108129