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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1177
+              if (!STI.hasMaskOperator())
+                report_fatal_error(
+                    "initialized packed aggregate with pointers '" +
----------------
tra wrote:
> 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.
> 
> We should probably have a test case triggering this.
The first test in `packed-aggr.ll` checks the error.

> Also, we may want to move the check into printInBytes, as otherwise it may be possible to invoke it when mask() is not supported.
That would require passing `GVar` and `STI` there, only to check the requirements and to print the error message. Not sure it's worth it.


================
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(";
----------------
tra wrote:
> 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.
Done in D129773


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

https://reviews.llvm.org/D127504