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

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp:1197
           if (aggBuffer.numSymbols) {
-            if (static_cast<const NVPTXTargetMachine &>(TM).is64Bit()) {
+            if (Packed) {
+              if (!STI.hasMaskOperator())
----------------
ikudrin wrote:
> ikudrin wrote:
> > tra wrote:
> > > What will happen if we have an aggregate with a mix of packed and unpacked fields and we only want to init with a pointer located in the unpacked part of it? Will something like this trigger the error?
> > > ```
> > > %ti = type <{ i8, i32 }>
> > > %to = type { i8, %ti, i32, void ()* }
> > > @n = addrspace(1) global %to {
> > >     i8 12,
> > >     %ti <{ i8 56, i32 78 }>,
> > >     i32 34,
> > >     void()* @func
> > >     }
> > > ```
> > > 
> > > It's one of the test cases below with another pointer added to non-packed top-level struct.
> > > 
> > Unfortunately, yes, that would trigger the error. On the other hand, supporting that case would require a deeper analysis of the initialization expression itself, not only the type, because there can be `ptrtoint` operators. I'm not sure if we need that complication, considering that CUDA 11.1 is almost 2 years old already.
> > Unfortunately, yes, that would trigger the error. On the other hand, supporting that case would require a deeper analysis of the initialization expression itself, not only the type, because there can be `ptrtoint` operators. I'm not sure if we need that complication, considering that CUDA 11.1 is almost 2 years old already.
> 
> 
I think we need to fix this. I'm not convinced that recursively checking fields'  'packed'-ness makes sense here.

I think we only care whether the structure with the field we need to init is packed or not. If it's not packed, then per-field alignment guarantees still apply and that's all we need.
E.g. in the example above, it does not matter whether any of the fields itself are packed or not. The pointer field alignment is only affected by its containing aggregate itself.

Perhaps I'm missing something. If we do need recursive check, could you give me an example where that would make a difference?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127504