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

[Justin Lebar via llvm-commits]

jlebar added a comment.

I had a few minutes to read through the comment here, hope you don't mind the drive-by review.  I thought the comment was really helpful, btw.

Would it be worth adding something to the pass this one is intended to replace (and maybe this one as well) to indicate the relationship between the two passes?  I expect this may be unclear to someone just reading the code.


================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:16
@@ +15,3 @@
+//
+// Unfortunately, type qualifiers only apply to variable declarations so CUDA
+// compilers must infer the memory space of an address expression from
----------------
Nit: s/ so/, so/

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:55
@@ +54,3 @@
+//
+// The major challenge of implementing this optimization is handling PHINodes
+// which may create loops in the data flow graph. This brings two complications.
----------------
I think you want a comma after "PHINodes", since (I think) this is a nonrestrictive clause.  Meaning, you're saying that phi nodes are the challenge, not that specifically phi nodes that create loops are the challenge.

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:78
@@ +77,3 @@
+// Second, IR rewriting in Step 2 also needs to be circular. For example,
+// converting %y to addrspace(3) requires to know the converted %y2, but
+// converting %y2 needs the converted %y. To address this complication, we break
----------------
Nit: "requires us to know" or "requires the compiler to know".

================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:86
@@ +85,3 @@
+//   %y' = phi float addrspace(3)* [ %input, undef ]
+// Then, convert %y2 to
+//   %y2' = getelementptr %y', 1
----------------
Nit: List parallelism.  "Our algorithm first converts a to b.  Then it *converts* b to c.  FInally, it *fixes* the undef in c so that"


http://reviews.llvm.org/D17965