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

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

LGTM with few nits.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1275
+    }
+    pos += ptrSize - 1;
+    nextSymbolPos = symbolPosInBuffer[++nSym];
----------------
Nit: splitting pos incrementing between the for loop and the body looks a bit odd.

It may be a bit better to increment pos where we actually produce output:
+=1 where we do byte-by-byte output and += ptrSize where we output whole pointer.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.h:85
+    bool allSymbolsAligned(unsigned ptrSize) const {
+      return std::all_of(symbolPosInBuffer.begin(), symbolPosInBuffer.end(),
+                         [=](unsigned pos) { return pos % ptrSize == 0; });
----------------
Nit: we would use `llvm::all_of(symbolPosInBuffer, ...)` and make it a bit more concise.


================
Comment at: llvm/test/CodeGen/NVPTX/packed-aggr.ll:3
+; RUN:   FileCheck %s --check-prefix=ERR
+; ERR: initialized packed aggregate with pointers 's1' requires at least PTX ISA version 7.1
+
----------------
I've missed this test case when I reviewed previous revision of this patch. `ERR` and `RUN` kind of blend together. 
Maybe change it to `ERROR` to make it easier to see.
I'd also move it down to where the `s1` is defined. 



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

https://reviews.llvm.org/D127504