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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:493
+
+  /// Return true if particular instruction (assumed to be a memory access
+  /// instruction) has volatile variant. If that's the case then we can avoid
----------------
s/particular/the particular/ or perhaps better /the given/


================
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;
----------------
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.  :)


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:712
 /// a single pointer operand that can have its address space changed by simply
 /// mutating the use to a new value.
+static bool isSimplePointerUseValidToReplace(const TargetTransformInfo &TTI,
----------------
Suggest adding a comment, since this is tricky.  Maybe "If the memory instruction is volatile, return true only if the target allows the memory instruction to be volatile in the new address space."


================
Comment at: llvm/test/CodeGen/NVPTX/ld-st-addrrspace.py:59
+      [True, False],
+      ["", ".shared", ".global", ".const", ".local", ".param"]):
+
----------------
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.


================
Comment at: llvm/test/CodeGen/NVPTX/ld-st-addrrspace.py:73
+    if volatile and not space in ["",".global",".shared"]:
+      continue
+
----------------
Maybe this would be blearer if we moved it above the params dict?

Also, spaces after the commas.  :)  There *are* pyformatters out there, e.g. https://github.com/myint/pyformat (first hit when randomly searching), which looks like it's actually just a wrapper around https://pypi.python.org/pypi/autopep8.


================
Comment at: llvm/test/CodeGen/NVPTX/ld-st-addrrspace.py:76
+    test_params = params
+    print(Template(load_template).substitute(test_params))
+
----------------
This test_params vs. params difference looks left over from the previous python script.  Here it seems unnecessary?


https://reviews.llvm.org/D39026