[PATCH] D142060: [DAGCombine] Slightly improve perf of SimplifySelectOps

[Keno Fischer via Phabricator via llvm-commits]

loladiro created this revision.
loladiro added a reviewer: niravd.
Herald added subscribers: ecnelises, hiraditya.
Herald added a project: All.
loladiro requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

In [1], I noted an issue with SimplifySelectOps' performance.
This revision contains a few misc changes that improve
performance slightly by avoiding unnecessary walks, but
because the overall problem reported in this issue is algorithmic,
the end-to-end perf impact is minimal (at most 3x faster or so).

In particular, this change:

- Removes redundant `->isPredecessorOf` check that is covered more performantly by the subsequent call to `hasPredecessorHelper`.

- Changes the backwards search barrier from `TheSelect` (which, contrary to the comment above, should always be a *successor* of the nodes in question). Instead, we use the chain operand of the loads. One of the legality conditions is that both loads have the same chain operand, so this is guaranteed to be a predecessor of at least the loads, and cuts off search through it.

- Corrects the argument checked in `hasAnyUseOfValue`. According to the comment, this was supposed to check the chain operand of the load. However, it instead checks the value operand is thus trivially `true`, because we know that the value is used by `TheSelect`. I think the comment is correct here. We already know that `TheSelect` is the only user of the loads' values. As a result, the only path to the condition would have to go through the chain operand, so if that is unused, we can avoid performing the search.

[1] https://github.com/llvm/llvm-project/issues/60132


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142060

Files:
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp


Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -24969,21 +24969,16 @@
                                       LLD->getBasePtr().getValueType()))
       return false;
 
-    // The loads must not depend on one another.
-    if (LLD->isPredecessorOf(RLD) || RLD->isPredecessorOf(LLD))
-      return false;
-
     // Check that the select condition doesn't reach either load.  If so,
     // folding this will induce a cycle into the DAG.  If not, this is safe to
     // xform, so create a select of the addresses.
-
     SmallPtrSet<const SDNode *, 32> Visited;
     SmallVector<const SDNode *, 16> Worklist;
 
-    // Always fail if LLD and RLD are not independent. TheSelect is a
-    // predecessor to all Nodes in question so we need not search past it.
+    // The token chain was verified to be identical between RHS and LHD above
+    // and transitively is a predecessor of all nodes in question.
+    Visited.insert(LHS.getOperand(0).getNode());
 
-    Visited.insert(TheSelect);
     Worklist.push_back(LLD);
     Worklist.push_back(RLD);
 
@@ -24997,13 +24992,14 @@
       // predecessor to {RLD, LLD, CondNode}. As we've already compared the
       // Loads, we only need to check if CondNode is a successor to one of the
       // loads. We can further avoid this if there's no use of their chain
-      // value.
+      // value, because we check about that the only use of the value result
+      // is the select itself.
       SDNode *CondNode = TheSelect->getOperand(0).getNode();
       Worklist.push_back(CondNode);
 
-      if ((LLD->hasAnyUseOfValue(1) &&
+      if ((LLD->hasAnyUseOfValue(0) &&
            SDNode::hasPredecessorHelper(LLD, Visited, Worklist)) ||
-          (RLD->hasAnyUseOfValue(1) &&
+          (RLD->hasAnyUseOfValue(0) &&
            SDNode::hasPredecessorHelper(RLD, Visited, Worklist)))
         return false;
 
@@ -25023,9 +25019,9 @@
       Worklist.push_back(CondLHS);
       Worklist.push_back(CondRHS);
 
-      if ((LLD->hasAnyUseOfValue(1) &&
+      if ((LLD->hasAnyUseOfValue(0) &&
            SDNode::hasPredecessorHelper(LLD, Visited, Worklist)) ||
-          (RLD->hasAnyUseOfValue(1) &&
+          (RLD->hasAnyUseOfValue(0) &&
            SDNode::hasPredecessorHelper(RLD, Visited, Worklist)))
         return false;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142060.490304.patch
Type: text/x-patch
Size: 2453 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230118/17a404d1/attachment.bin>