[PATCH] D78272: [DAGCombiner] Combine shifts into multiply-high

Tue Jun 2 05:31:29 PDT 2020

nemanjai added a comment.

LGTM. My only remaining comments are nits about early exits from the function not happening as early as they can, but such reordering is trivial and can happen on the commit. Thank you.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7947
+  // legal for NarrowVT on the target.
+  if (!TLI.isMulhCheaperThanMulShift(NarrowVT))
+      return SDValue();
----------------
I think the check for the types of the inputs to the two extend nodes belongs here. Generally favour early exits as soon as the necessary information is available.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7959
+  // of the narrow type.
+  ConstantSDNode *ShiftAmtSrc = isConstOrConstSplat(N->getOperand(1));
+  if (!ShiftAmtSrc)
----------------
This check is on the outermost operation (i.e. the shift). We can probably move this early exit towards the very top of the function as it makes no sense to do any other checks if the shift amount isn't constant.
I don't think another round of review is required for this though - feel free to address this on the commit.

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

https://reviews.llvm.org/D78272