[DAGCombine] Allow GatherAllAliases to pass through inline asm calls and glued nodes.

[niravd]

Summary

Teach GatherAllAliases to reason about inline assembly nodes and
associated glued CopyToReg/CopyFromReg nodes.

This fixes PR9517.

[niravd]

Actual Patch.

[niravd]

This is a refactored and rebased version of D49691 so that it looks reasonable on Phabricator. I've factored out a initial NFC patch (the first snapshot in history) to highlight the actual functional change.

[niravd]

Ping.

Just a reminder to ease review: The first diff in the history is an extracted NFC refactoring and difference between Diffs 1 and 2 highlight the actual change.

[efriedma]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

19428

"If this had a glued output, return C"?

I don't follow why it matters if the output is glued, given there isn't a glue input. If I'm understanding correctly, if the copy isn't glued, in it doesn't modify memory. If the copy is glued, the only way to reach this case should be through a recursive call of ImproveChain. Or are you just trying to be conservative here?

[niravd]

Realized explanatory comment and associate fixup had been lost in patch stack. Folding it back in.

[efriedma]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

19447

In terms of the actual checks here, is this sufficient to catch any inline asm that accesses memory? Do you need to check for Extra_HasSideEffects? Do you need to check for "indirect" operands separately? (I think it's possible to write an indirect register operand in IR, although I'm not sure clang ever generates them...)

[niravd]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

19447

SelectionDAGBuilder updates the inlineasm's ExtraInfo based on the asm parameter constraints so checking extra info should be should be sufficient.

Also, a check for HasExtraSideEffects is probably needed. I've updated the patch as such.

[niravd]

Check HasSideEffects in analysis for inlineasm.

[efriedma]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

19447

With the change to check HasExtraSideEffects, can you remove the Mips test changes?

[niravd]

Revert mips test changes

[efriedma]

LGTM

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (UseAA && AA &&
     if (AAResult == NoAlias)
       return false;
   }
 
   // Otherwise we have to assume they alias.
   return true;
 }
 
-/// Walk up chain skipping non-aliasing memory nodes,
-/// looking for aliasing nodes and adding them to the Aliases vector.
 void DAGCombiner::GatherAllAliases(LSBaseSDNode *N, SDValue OriginalChain,
                                    SmallVectorImpl<SDValue> &Aliases) {
   SmallVector<SDValue, 8> Chains;     // List of chains to visit.
   SmallPtrSet<SDNode *, 16> Visited;  // Visited node set.
 
   // Get alias information for node.
-  bool IsLoad = isa<LoadSDNode>(N) && !N->isVolatile();
+  const bool IsLoad = isa<LoadSDNode>(N) && !N->isVolatile();
 
   // Starting off.
   Chains.push_back(OriginalChain);
   unsigned Depth = 0;
 
+  // Attempt to improve chain by a single step
+  std::function<bool(SDValue &)> ImproveChain = [&](SDValue &C) -> bool {
+    switch (C.getOpcode()) {
+    case ISD::EntryToken:
+      // No need to mark EntryToken.
+      C = SDValue();
+      return true;
+    case ISD::LOAD:
+    case ISD::STORE: {
+      // Get alias information for C.
+      auto LSChain = cast<LSBaseSDNode>(C.getNode());
+      bool IsOpLoad = isa<LoadSDNode>(C.getNode()) && !LSChain->isVolatile();
+      if ((IsLoad && IsOpLoad) || !isAlias(N, LSChain)) {
+        // Look further up the chain.
+        C = C.getOperand(0);
+        return true;
+      }
+      // Alias, so stop here.
+      return false;
+    }
+
+    case ISD::CopyFromReg:
+      // Always forward past past CopyFromReg.
+      C = C.getOperand(0);
+      return true;
+    default:
+      return false;
+    }
+  };
+
   // Look at each chain and determine if it is an alias.  If so, add it to the
   // aliases list.  If not, then continue up the chain looking for the next
   // candidate.
   while (!Chains.empty()) {
     SDValue Chain = Chains.pop_back_val();
 
+    // Don't bother if we've seen Chain before.
+    if (!Visited.insert(Chain.getNode()).second)
+      continue;
+
     // For TokenFactor nodes, look at each operand and only continue up the
     // chain until we reach the depth limit.
     //
     // FIXME: The depth check could be made to return the last non-aliasing
     // chain we found before we hit a tokenfactor rather than the original
     // chain.
     if (Depth > TLI.getGatherAllAliasesMaxDepth()) {
       Aliases.clear();
       Aliases.push_back(OriginalChain);
       return;
     }
 
-    // Don't bother if we've been before.
-    if (!Visited.insert(Chain.getNode()).second)
-      continue;
-
-    switch (Chain.getOpcode()) {
-    case ISD::EntryToken:
-      // Entry token is ideal chain operand, but handled in FindBetterChain.
-      break;
-
-    case ISD::LOAD:
-    case ISD::STORE: {
-      // Get alias information for Chain.
-      bool IsOpLoad = isa<LoadSDNode>(Chain.getNode()) &&
-          !cast<LSBaseSDNode>(Chain.getNode())->isVolatile();
-
-      // If chain is alias then stop here.
-      if (!(IsLoad && IsOpLoad) &&
-          isAlias(N, cast<LSBaseSDNode>(Chain.getNode()))) {
-        Aliases.push_back(Chain);
-      } else {
-        // Look further up the chain.
-        Chains.push_back(Chain.getOperand(0));
-        ++Depth;
-      }
-      break;
-    }
-
-    case ISD::TokenFactor:
+    if (Chain.getOpcode() == ISD::TokenFactor) {
       // We have to check each of the operands of the token factor for "small"
       // token factors, so we queue them up.  Adding the operands to the queue
       // (stack) in reverse order maintains the original order and increases the
       // likelihood that getNode will find a matching token factor (CSE.)
       if (Chain.getNumOperands() > 16) {
         Aliases.push_back(Chain);
-        break;
+        continue;
       }
       for (unsigned n = Chain.getNumOperands(); n;)
         Chains.push_back(Chain.getOperand(--n));
       ++Depth;
-      break;
-
-    case ISD::CopyFromReg:
-      // Forward past CopyFromReg.
-      Chains.push_back(Chain.getOperand(0));
+      continue;
+    }
+    // Everything else
+    if (ImproveChain(Chain)) {
+      // Updated Chain Found, Consider new chain if one exists.
+      if (Chain.getNode())
+        Chains.push_back(Chain);
       ++Depth;
-      break;
-
-    default:
-      // For all other instructions we will just have to take what we can get.
-      Aliases.push_back(Chain);
-      break;
+      continue;
     }
+    // No Improved Chain Possible, treat as Alias.
+    Aliases.push_back(Chain);
   }
 }
 
 /// Walk up chain skipping non-aliasing memory nodes, looking for a better chain
 /// (aliasing node.)
 SDValue DAGCombiner::FindBetterChain(LSBaseSDNode *N, SDValue OldChain) {
   if (OptLevel == CodeGenOpt::None)
     return OldChain;