[PATCH] D49691: [DAGCombine] Allow alias analysis with inline asm calls and GluedNodes.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'm not sure I like the algorithm here.  It seems like it would be more straightforward to handle the INLINEASM and all the glued nodes together, in a separate inner loop.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18870
 
-    case ISD::CopyFromReg:
+      case ISD::CopyFromReg: {
       // Forward past CopyFromReg.
----------------
Wrong indentation.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18875
+                     Chain->getOperand(0).getNode() == Chain->getOperand(LastOpNo).getNode();
+      //  bool isGlued = Chain->getOperand(Chain->getNumOperands()-1).getValueType() == MVT::Glue;
+
----------------
Commented-out code?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18883
+      case ISD::CopyToReg: {
+      // Forward past CopyToRe
+      auto LastOpNo = Chain->getNumOperands()-1;
----------------
Incomplete comment?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18918
       // For all other instructions we will just have to take what we can get.
-      Aliases.push_back(Chain);
+      Visited.insert(Chain->getOperand(0).getNode()); 
+      Aliases.push_back(Alias);
----------------
I don't think there's an comment anywhere explaining why you're putting chain operands into the visited list? Thinking about it a bit, I guess you're trying to avoid computing "useless" aliases?  Does that actually work reliably, given the visitation order of the operands of a TokenFactor is sort of arbitrary?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49691