[PATCH] D111530: [TargetLowering] Optimize expanded SRL/SHL fed into SETCC ne/eq 0

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3399
+// rotation and it could be eliminated as far as the overall result is compared
+// with zero.
+//
----------------
Lots of comment duplication - description to header, and keep the code snippet here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3440
+  auto MatchShifts = [&UnmatchedShifts, &Result, &MatchedShiftsCount,
+                      OpSizeInBits](SDValue &Op, APInt C, bool IsLeft) {
+    ShiftInfo &Info = UnmatchedShifts[Op];
----------------
const APInt &C


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3476
+             Scan(Value->getOperand(1), HeightLimit - 1);
+    } else if (Opcode == ISD::SRL || Opcode == ISD::SHL) {
+      if (!(C = dyn_cast<ConstantSDNode>(Value->getOperand(1))))
----------------
(style) Break apart if-else chains when they return.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3486
+      SDValue Op2 = Value->getOperand(1);
+      APInt CVal = C->getAPIntValue();
+      // For funnel shifts second operand is effectively shifted
----------------
const APInt &CVal = 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3497
+  // has a height of 8, so it should be enough to cover most practical cases.
+  constexpr unsigned MaxTreeHeight = 8;
+  if (!Scan(N0, MaxTreeHeight))
----------------
Why was i4096 of particular interest?

We have a default depth limit we use for this kind of thing: SelectionDAG::MaxRecursionDepth = 6, also, normally we increment a depth to that value instead of decrementing a height.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3508
+  if (!UnmatchedShifts.empty()) {
+    auto UnmatchedShift = UnmatchedShifts.begin();
+    SDValue Op = UnmatchedShift->first;
----------------
(style) Don't use auto


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3517
+  // Reduce all values using OR.
+  for (size_t Index = 0; Index + 1 < Result.size(); Index += 2) {
+    SDValue NewOr = DAG.getNode(ISD::OR, DL, N0.getValueType(), Result[Index],
----------------
Can't you avoid pushing results by just OR'ing the results once?
```
SDValue Reduction = Result[0];
for (size_t I = 1, E = Results.size(); I < E; ++I)
  Reduction = DAG.getNode(ISD::OR, DL, N0.getValueType(), Reduction, Result[I]);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111530