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

[Jingyue Wu via llvm-commits]

jingyue added a comment.

Thanks a lot for the review! I understand it's a lot of work :) I answered one of your high-level questions. I'm still OOO and will get to other comments later.


================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:313
@@ +312,3 @@
+  }
+}
+
----------------
jlebar wrote:
> I may be missing something, but it seems like this would be a lot simpler if we instead cloned the old instruction, then iterated over its relevant operands and converted the pointer ones to the new address space.  Then we wouldn't have to worry about tricky things like remembering to call setInBounds.
> 
> In fact if we wanted, I think we could write this mostly-generically:
> 
>   * for each instruction type, make a list of all of the operand indices that may contain pointers and that we want to modify.
>   * then iterate over that list and convert the ones which are pointers to the new address space.
I tried that initially. The problem was you can't just "convert" the address space of a pointer operand of a `Value` without changing the type of the `Value`. e.g.,
```
float* a = GEP float, float* b, 0, 1
```
if we convert `b`'s type to `float addrspace(3)*`, the `GEP` instruction becomes invalid because `a`'s type is still `float*`. We would need an extra step of `a->mutateType()` which is discouraged and doesn't work for constant expressions (footnote: the lookup table for constant expressions uses type + operands as keys, so mutating the types brutally would mess it up.)

So, I chose to construct new instructions and constant expressions instead of directly converting types, as it is safer and does not depend too much on the internals. 


http://reviews.llvm.org/D17965