[PATCH] D67021: [DAGCombiner] improve throughput of shift+logic+shift

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7251-7252
+  const APInt *C0;
+  if (matchFirstShift(LogicOp.getOperand(0), C0)) {
+    X = LogicOp.getOperand(0).getOperand(0);
+    Y = LogicOp.getOperand(1);
----------------
lebedev.ri wrote:
> I think you want to make `matchFirstShift()` return that `X`.
That looked more awkward to me than capturing the shifted value by reference along with the shift amount.
See if the updated code is easier to read.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7266
+  SDValue NewShift1 = DAG.getNode(ShiftOpcode, DL, VT, X, ShiftSum);
+  SDValue NewShift2 = DAG.getNode(ShiftOpcode, DL, VT, Y, Shift->getOperand(1));
+  return DAG.getNode(LogicOpcode, DL, VT, NewShift1, NewShift2);
----------------
lebedev.ri wrote:
> It would be less confusing to `C1 = Shift->getOperand(1);` somewhere up there, and use `C1` here.
I've never found a good way to juggle the naming of the various SDValue, ConstantSDNode, and underlying APInt variables. See if the update is easier to read.


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

https://reviews.llvm.org/D67021