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

[Jingyue Wu via llvm-commits]

jingyue added inline comments.

================
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);
----------------
jlebar wrote:
> Is this change related to the new pass we're adding?  Not clear to me how...
NVPTXInferAddressSpaces does a better job in preserving the variable names than the old version of NVPTXFavorNonGenericAddrSpaces. After this change, the two passes can share the same `@rauw` test. 

In the old version, `NewASC` is likely going to be removed, so it makes more sense to assign the name to `NewGEP` instead. This makes this pass generate more named variables instead of something like `%1`. See `@rauw` in `access-non-generic.ll`. 

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:169
@@ +168,3 @@
+    return SmallVector<Value *, 2>(IncomingValues.begin(),
+                                   IncomingValues.end());
+  }
----------------
jlebar wrote:
> SmallVector has a constructor taking a range -- does that work here?
`PHINode::incoming_values` returns a range of `Use*` instead of a range of `Value*`. `Use*` can be type-converted to `Value*`, so my code works. 

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:272
@@ +271,3 @@
+    if (Src->getType() != NewPtrType)
+      return new BitCastInst(Src, NewPtrType);
+    return Src;
----------------
jlebar wrote:
> Maybe it's obvious, but I can't figure out when we hit this.  Src->getType()'s address space matches NewAddrSpace, but Src->getType() != NewPtrType?
`addrspacecast` can convert the pointee type as well. See `@ld_int_from_float`. 

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:413
@@ +412,3 @@
+namespace llvm {
+void initializeNVPTXInferAddressSpacesPass(PassRegistry &);
+}
----------------
jlebar wrote:
> Is there some reason not to define this one-liner here?
I don't get what you mean. What's the one-liner? 

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:419
@@ +418,3 @@
+
+bool NVPTXInferAddressSpaces::runOnFunction(Function &F) {
+  // Collects all generic address expressions in postorder.
----------------
jlebar wrote:
> I haven't written any large passes, so if this is wrong, feel free to ignore, but carrying the InferredAddressSpaces map as a member variable seems like it might add more complexity than using it as a local variable here.  In particular, we wouldn't need to remember to clear it, and updateAddressSpace could simply return the new address space, instead of updating the global map.  (Maybe it would return an Optional<unsigned>, or it could just return the same address space to indicate "no change".)
I like your proposal. PTAL. 

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

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:538
@@ +537,3 @@
+      for (Use &U : V->uses())
+        Uses.push_back(&U);
+      DEBUG(dbgs() << "Replacing the uses of " << *V << "\n  to\n  " << *NewV
----------------
jlebar wrote:
> Why can't we iterate over V->uses() directly?  (Is it because we're modifying them?)
Yes. `U->set(...)` invalidates use iterators of `U->getUser()`. 

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:560
@@ +559,3 @@
+            BasicBlock::iterator InsertPos = std::next(I->getIterator());
+            while (isa<PHINode>(InsertPos))
+              ++InsertPos;
----------------
jlebar wrote:
> Maybe it's obvious, but it's not clear to me why we want to go past phi nodes.  We're adding a cast to the value V, which is defined at position I->getIterator().  So surely that value is in fact available right after that position -- why skip further down?
In LLVM IR, PHINodes have to be in the front of a basic block. 

================
Comment at: test/CodeGen/NVPTX/access-non-generic.ll:6
@@ -4,1 +5,3 @@
+; RUN: opt < %s -S -nvptx-infer-addrspace | FileCheck %s --check-prefix IR
+; RUN: opt < %s -S -nvptx-infer-addrspace | FileCheck %s --check-prefix IR-WITH-LOOP
 
----------------
jlebar wrote:
> Are these incompatible -- i.e., we can't combine these two into one FileCheck call with two use-prefixes?
IR-WITH-LOOP only works with `nvptx-infer-addrspace` and not `nvptx-favor-non-generic`, so I split the tests into

IR: check both favor and infer
IR-WITH-LOOP: check only favor


http://reviews.llvm.org/D17965