[Openmp-commits] [PATCH] D122550: [NVPTX] Allow degenerated `nvvm.annotations`

[Daniil Kovalev via Phabricator via Openmp-commits]

kovdan01 added a comment.

Thanks @jdoerfert! I was going to implement a fix for that, but have not managed to do it yet. See a couple of inline comments below.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:4273
       for (const MDNode *MDN : NMDN->operands()) {
         assert(MDN->getNumOperands() == 3);
 
----------------
In D120129, you added a comment containing the following piece of IR:

```
!nvvm.annotations = !{!8, !9, !8, !10, !10, !10, !10, !11, !11, !10}

!8 = !{null, !"align", i32 8}
!9 = !{null, !"align", i32 8, !"align", i32 65544, !"align", i32 131080}
!10 = !{null, !"align", i32 16}
!11 = !{null, !"align", i32 16, !"align", i32 65552, !"align", i32 131088}
```

According to that, one piece of metadata can contain more than 3 operands. I was unable to find a piece of code where we create such piece of metadata. I even was ubable to find code that adds align info to nvvm.annotations. All that I have found – `NVPTXTargetCodeGenInfo::addNVVMMetadata` and similar/duplicating code, that creates a piece of metadata consisting of 3 operands. So, I can’t state for sure without seeing code responsible for that, but given your IR above, a question arises: shouldn’t we change this assert?


================
Comment at: llvm/test/CodeGen/NVPTX/param-vectorize-device.ll:803
+
+!nvvm.annotations = !{!0}
+
----------------
I think that we should add a comment explaining what is the purpose of these lines. It might be unclear out of the context of the revision. See `; Section X - checking that...` comments above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122550