[PATCH] D58009: [DAGCombine] Simplify funnel shifts with undef args to bitshifts

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some thoughts.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7119
   SDValue N1 = N->getOperand(1);
   SDValue N2 = N->getOperand(2);
   bool IsFSHL = N->getOpcode() == ISD::FSHL;
----------------
What should be done if N2 is `undef`?
Pretend that it is `0`, or replace the entire op with `undef`?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7148
+    // fold fshr(N0, undef, C) -> lshr(N0, BW-C)
+    if (N0.isUndef())
+      return DAG.getNode(ISD::SRL, SDLoc(N), VT, N1,
----------------
Since we can replace `undef` with something, e.g. `0`, we should do the same if it's `0` too.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7152
+                                         SDLoc(N), ShAmtTy));
+    if (N1.isUndef())
+      return DAG.getNode(ISD::SHL, SDLoc(N), VT, N0,
----------------
Same


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7158-7159
+
+  // fold fshr(undef, N1, N2) -> lshr(N1, N2)
+  // fold fshl(N0, undef, N2) -> shl(N0, N2)
+  // TODO: when is it worth doing SUB(BW, N2) as well?
----------------
Do `ISD::SRL` / `ISD::SHL` implicitly take the modulo of the shift amount?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7161
+  // TODO: when is it worth doing SUB(BW, N2) as well?
+  if (N0.isUndef() && !IsFSHL)
+    return DAG.getNode(ISD::SRL, SDLoc(N), VT, N1, N2);
----------------
Same remark re `0`.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7163
+    return DAG.getNode(ISD::SRL, SDLoc(N), VT, N1, N2);
+  if (N1.isUndef() && IsFSHL)
+    return DAG.getNode(ISD::SHL, SDLoc(N), VT, N0, N2);
----------------
Same remark re `0`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58009