[PATCH] D17965: [NVPTX] Adds a new address space inference pass.

[Justin Lebar via llvm-commits]

jlebar added a comment.

Slowly making my way through this, here's a checkpoint.  So far I'm finding this quite easy to read.  Thank you!


================
Comment at: lib/Target/NVPTX/NVPTXFavorNonGenericAddrSpaces.cpp:167
@@ -166,2 +166,3 @@
     NewGEP->setIsInBounds(GEP->isInBounds());
+    NewGEP->takeName(GEP);
     NewASC = new AddrSpaceCastInst(NewGEP, GEP->getType(), "", GEPI);
----------------
Is this change related to the new pass we're adding?  Not clear to me how...

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:144
@@ +143,3 @@
+// getelementptr operators.
+bool isAddressExpression(const Value &V) {
+  if (!isa<Operator>(V))
----------------
As I read the style guide, we're supposed to make anonymous namespaces as small as possible and then use static functions everywhere else?  http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:169
@@ +168,3 @@
+    return SmallVector<Value *, 2>(IncomingValues.begin(),
+                                   IncomingValues.end());
+  }
----------------
SmallVector has a constructor taking a range -- does that work here?

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:174
@@ +173,3 @@
+  case Instruction::GetElementPtr:
+    return SmallVector<Value *, 2>(1, Op.getOperand(0));
+  default:
----------------
Nit: SmallVector has an initializer_list constructor, so would this be clearer if we just said

  return {Op.getOperand(0)};

or

  return SmallVector<Value *, 2>{{Op.getOperand(0)}};

?  For my part, I can never remember which way the args in the constructor here go.

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:199
@@ +198,3 @@
+  // use-def graph of function F.
+  //
+  // The stack used for postorder traversal. The first item of each pair is the
----------------
Uber nit: I think not having this "//" would be helpful, as the two comments are entirely different, rather than two paragraphs in a single comment.  As-is I had to backtrack to understand what was happening.

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:220
@@ +219,3 @@
+  while (!PostorderStack.empty()) {
+    // If the operands of the expression on the top is already explored,
+    // adds that expression to the resultant postorder.
----------------
s/is/are/

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:413
@@ +412,3 @@
+namespace llvm {
+void initializeNVPTXInferAddressSpacesPass(PassRegistry &);
+}
----------------
Is there some reason not to define this one-liner here?

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:423
@@ +422,3 @@
+
+  // Runs a data-flow analysis to refine the address spaces of every expressions
+  // in Postorder.
----------------
every expression

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:435
@@ +434,3 @@
+  std::vector<Value*> Worklist(Postorder);
+  DenseSet<Value *> InWorklist;
+  InWorklist.insert(Worklist.begin(), Worklist.end());
----------------
Does SetVector work for you here?

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:437
@@ +436,3 @@
+  InWorklist.insert(Worklist.begin(), Worklist.end());
+  // Initially, every expressions are in the uninitialized address space.
+  InferredAddrSpace.clear();
----------------
every expression is in


http://reviews.llvm.org/D17965