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

[Justin Lebar via llvm-commits]

jlebar accepted this revision.
jlebar added a comment.
This revision is now accepted and ready to land.

Thank you very much for explaining the bits of this that were confusing to me -- it's very helpful!


================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:413
@@ +412,3 @@
+namespace llvm {
+void initializeNVPTXInferAddressSpacesPass(PassRegistry &);
+}
----------------
jingyue wrote:
> 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? 
I just mean  moving the definition of this function into this declaration.  Not a big deal either way.

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:419
@@ +418,3 @@
+
+bool NVPTXInferAddressSpaces::runOnFunction(Function &F) {
+  // Collects all generic address expressions in postorder.
----------------
jingyue wrote:
> 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. 
Looks good to me!

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:191
@@ +190,3 @@
+  case Instruction::GetElementPtr:
+    // return SmallVector<Value *, 2>(1, Op.getOperand(0));
+    return {Op.getOperand(0)};
----------------
I think you meant to remove the line above?

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:296
@@ +295,3 @@
+  SmallVector<Value *, 4> NewPointerOperands;
+  for (const Use& OperandUse : I->operands()) {
+    if (!OperandUse.get()->getType()->isPointerTy())
----------------
You'll probably want to go over this once more with clang-format to catch "&" and "*" placement.  I read too much google3 to notice this consistently, myself.  :)

================
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
 
----------------
I just meant

  FileCheck %s --check-prefix IR --check-prefix IR-WITH-LOOP

Not a big deal -- in fact maybe we'll get clearer errors as-is.


http://reviews.llvm.org/D17965