[PATCH] D39026: [NVPTX] allow address space inference for volatile loads/stores.

[Artem Belevich via Phabricator via llvm-commits]

tra marked 2 inline comments as done.
tra added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:495
+  /// instruction) has volatile variant. If that's the case then we can avoid
+  /// addrspacecast to generic AS for volatile loads/stores.
+  bool hasVolatileVariant(Instruction *I, unsigned AddrSpace) const;
----------------
jlebar wrote:
> I'm not sure this second sentence helps much -- it feels sort of apropos of nothing to me here.
> 
> Maybe we should say something to cover the fact that if this returns false, the instruction may still have a volatile variant -- we might simply not have implemented this TTI function for that target.  :)
Rephrased. PTAL.


================
Comment at: llvm/test/CodeGen/NVPTX/ld-st-addrrspace.py:59
+      [True, False],
+      ["", ".shared", ".global", ".const", ".local", ".param"]):
+
----------------
jlebar wrote:
> I'm not sure it's clearer, but ftr we could get rid of the three maps by doing
> 
> ```
> for (llvm_type, ptx_type), (llvm_addrspace, ptx_addrspace), volatile in product(
>   [("i8", "u8"), ...], [(0, ""), (1, ".global"), ...], [True, False]):
> ```
> 
> At least I'd recommend getting rid of get_as_id, since it's inconsistent with how we do the other two maps.
Fixed as_id. 
Folding three maps into a single statement is a bit too much python-fu, IMO. 


https://reviews.llvm.org/D39026