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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added reviewers: RKSimon, spatel.
frasercrmck added a comment.

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



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8525
+
+  bool HasConstant = LeftConstant || RightConstant;
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8583
 
-  SDValue Result = DAG.getNode(MulhOpcode, DL, NarrowVT, LeftOp.getOperand(0),
-                               RightOp.getOperand(0));
+  SDValue Result =
+      HasConstant
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108129