[PATCH] D47735: [DAGCombiner] Create rotates more aggressively

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

After thinking about it, i think it may make sense to have this after all.
I left some comments.

Also, it would be good to have some compile-time stats comparison here.
I really think you want do add some safety limits from the getgo.
E.g. would it make sense to have more than 8 levels of these `or`s?
More than 256 nodes? Etc.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5499-5500
+SDNode *DAGCombiner::ReassociateOrForRotate(SDValue Op0, SDValue Op1,
+                                            const SDLoc &dl) {
+  EVT VT = Op0.getValueType();
+  if (!hasOperation(ISD::ROTL, VT) && !hasOperation(ISD::ROTR, VT))
----------------
Do you want to assert that you start with `ISD::OR`?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5504
+
+  // Expand all single-use ORs into a list (OredOps).
+  SmallVector<SDValue,8> OredOps;
----------------
This comment should be next to the loop itself.
It would be best to explain the data structures here instead.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5516-5527
+    } else
+      OredOps.push_back(V);
+  }
+
+  // Since only shifts of the same SDValue can end up paired up into a rotate,
+  // create separate lists of shifts for each shifted value.
+  DenseMap<int,SmallVector<unsigned,8>> OpMap;
----------------
Can you not do this
```
if (Opc == ISD::SHL || Opc == ISD::SRL)
  OpMap[V.getOperand(0)->getNodeId()].push_back(I);
```
inside
```
    } else
      OredOps.push_back(V);
```
?

One less loop.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5521
+  // Since only shifts of the same SDValue can end up paired up into a rotate,
+  // create separate lists of shifts for each shifted value.
+  DenseMap<int,SmallVector<unsigned,8>> OpMap;
----------------
```
// for each shifted value, create a list of shifts.
```


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5522
+  // create separate lists of shifts for each shifted value.
+  DenseMap<int,SmallVector<unsigned,8>> OpMap;
+  for (unsigned I = 0, E = OredOps.size(); I != E; ++I) {
----------------
This data structure really needs a better name/description.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5538-5539
+
+  for (auto P : OpMap) {
+    auto &Shifts = P.second;
+    assert(!Shifts.empty() && "OpMap should not have empty lists");
----------------
I'm not sure whether or not `auto` is good here..


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5541
+    assert(!Shifts.empty() && "OpMap should not have empty lists");
+    llvm::sort(Shifts.begin(), Shifts.end(), OpcOrder);
+    // The list of shifts should only have SHL and SRL on it grouped into
----------------
Do we care about the order within those two groups? Is this still good in reverse-iteration mode?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5542-5545
+    // The list of shifts should only have SHL and SRL on it grouped into
+    // two contiguous segments. Find the beginning of the second segment.
+    auto Boundary = std::upper_bound(std::next(Shifts.begin()), Shifts.end(),
+                                     Shifts.front(), OpcOrder);
----------------
`llvm::partition_point()`?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5547-5549
+    for (unsigned I = 0, E = Boundary - Shifts.begin(); I != E; ++I) {
+      for (unsigned J = E, F = Shifts.size(); J != F; ++J) {
+        SDValue &OI = OredOps[Shifts[I]], &OJ = OredOps[Shifts[J]];
----------------
I'm not sure what is going on here, would be good to have a high-level description comment.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5552
+          continue;
+        if (SDNode *T = MatchRotate(OI, OJ, dl)) {
+          OredOps.push_back(SDValue(T, 0));
----------------
Early `continue`?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5569
+    return nullptr;
+
+  auto OredEnd = remove_if(OredOps, [](SDValue V) { return !bool(V); });
----------------
Likewise, this really needs to have a high-level description comment.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D47735