[PATCH] D120494: [NVPTX][AsmPrinter] Respect metadata 'align' for aggregate input parameters

Mon Mar 7 14:09:12 PST 2022

tra added inline comments.

================
Comment at: llvm/test/CodeGen/NVPTX/align-annotation.ll:27
+
+!5 = !{%struct.S1_t (%struct.S1_t)* @_Z5dummy4S1_t, !"align", i32 32, !"align", i32 65568}
----------------
krisb wrote:
> tra wrote:
> > Does clang generate this metadata?  AFAICT, it does not. https://godbolt.org/z/s9EYbdfjT
> > Where is this metadata expected to come from?
> > 
> Hmm, I looked at clang, and, surprisingly, it indeed doesn't generate `align` property, yet it generates all the other properties of `nvvm.annotations` metadata. We might add `align` to clang for the sake of completeness.
> However, clang support does not matter much in this context (it could be different frontends or users could write IR manually as `align` is defined in NVVM IR specification). It appears that the backend already supports the metadata; so this patch does not introduce a new one, it just makes the existing a bit more consistent.
> We might add align to clang for the sake of completeness.

Let me play a devil's advocate here. 

If we lived without it till now and there's no demonstrable benefit of having it supported, I'd argue that we do not need it and we should remove it instead.

NVVM metadata is a bit of a hack, IMO. Some of the things it's used for should be done via the standard LLVM mechanisms. E.g. kernels should be using a special calling convention, types do not have the concept of alignment in LLVM, only memory accesses do. What are we supposed to do with NVVM alignment info if the user-generated code provides conflicting alignment info about the loads/stores of the data? IMO it creates more problems than it was supposed to solve.

> it could be different frontends or users could write IR manually as align is defined in NVVM IR specification

NVVM IR specification is somewhat irrelevant here. LLVM != NVVM. We have already diverged quite a bit since the time NVIDIA has contributed NVPTX back-end.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120494