[PATCH] D127504: [NVPTX] Use the mask() operator to initialize packed structs with pointers

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Nice. I like this implementation better.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1177
+              if (!STI.hasMaskOperator())
+                report_fatal_error(
+                    "initialized packed aggregate with pointers '" +
----------------
We should probably have a test case triggering this.

Also, we may want to move the check into `printInBytes`, as otherwise it may be possible to invoke it when mask() is not supported.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1180
+                    GVar->getName() +
+                    "' requires the mask() operator which is supported from "
+                    "PTX ISA version 7.1");
----------------
`requires at least`


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1234-1235
+    // Is v0 a non-generic pointer?
+    bool isNonGenericPointer = PTy && PTy->getAddressSpace() != 0;
+    if (EmitGeneric && !isa<Function>(v) && !isNonGenericPointer) {
+      os << "generic(";
----------------
It may be a bit easier to read this way:
```
  bool isGenericPointer = PTy && PTy->getAddressSpace() == 0;
    if (EmitGeneric  && isGenericPointer && !isa<Function>(v)) {
       // wrap in generic();
   } else {
       ...
   }
```
Comment above would need to be updated, too.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1264
+    printSymbol(nSym, oss);
+    for (unsigned i = 0; i < ptrSize; ++i) {
+      if (i)
----------------
I'd add a comment that we're generating a per-byte `mask` operator here with an example illustrating it.
PTX's `mask()` is weird due to the fact that it actually has no `mask` in it, so an explanation would be useful here.




================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.h:84-87
+    bool allSymbolsAligned(unsigned ptrSize) const {
+      return std::none_of(symbolPosInBuffer.begin(), symbolPosInBuffer.end(),
+                          [=](unsigned pos) -> bool { return pos % ptrSize; });
+    }
----------------
I'd suggest rephrasing it as "all are aligned", which would closer match our intent here vs "none are non-aligned" we have now which is equivalent, but is a bit harder to read.

`std::all_of(  ... return pos % ptrSize == 0; )`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127504/new/

https://reviews.llvm.org/D127504