[PATCH] D29164: NVPTX: Refactor NVPTXInferAddressSpaces to check TTI

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:230
+  /// For targets with different pointer representations which can be converted
+  /// with the addrspacecast instruction. Returns the address space ID value for
+  /// a target's 'generic' address space. The 'generic' address space in this
----------------
Nit, can we rephrase so the first sentence (a) says what the function does and (b) isn't a fragment?


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:230
+  /// For targets with different pointer representations which can be converted
+  /// with the addrspacecast instruction. Returns the address space ID value for
+  /// a target's 'generic' address space. The 'generic' address space in this
----------------
jlebar wrote:
> Nit, can we rephrase so the first sentence (a) says what the function does and (b) isn't a fragment?
s/ID value/ID/?


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:233
+  /// context means a pointer where access of a memory location through a
+  /// pointer with this address space is expected to be undesirable compared to
+  /// the same memory location accessed through a pointer with a different
----------------
Should we say "legal but undesirable"?  At which point should we say what we really mean, which is "slower than"?


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:236
+  /// address space.
+  unsigned getUndesirableAddressSpace() const;
+
----------------
Maybe I'm biased because I'm coming from the CUDA world, but "undesirable" doesn't really seem to capture what's going on as well as 'generic'., and indeed the comment uses "generic" to good effect.

What are you trying to get across with the name, exactly?


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:236
+  /// address space.
+  unsigned getUndesirableAddressSpace() const;
+
----------------
jlebar wrote:
> Maybe I'm biased because I'm coming from the CUDA world, but "undesirable" doesn't really seem to capture what's going on as well as 'generic'., and indeed the comment uses "generic" to good effect.
> 
> What are you trying to get across with the name, exactly?
Maybe this should return optional<unsigned>?


================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:444
+  if (GenericAddrSpace == ADDRESS_SPACE_UNINITIALIZED)
+    return false;
+
----------------
If we used optional<unsigned> we wouldn't need this hack, either.


https://reviews.llvm.org/D29164