[PATCH] D127504: [NVPTX] Use the mask() operator to initialize packed structs with pointers
Artem Belevich via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 15 11:43:31 PDT 2022
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
More information about the llvm-commits
mailing list