[PATCH] D115888: [Attributor][Fix] Add alignment return attribute to HeapToStack

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1411
+        CGM.getLLVMContext(), llvm::Attribute::Alignment,
+        CGM.getContext().getTypeAlignInChars(VarTy).getQuantity()));
 
----------------
This doesn't work. If the type alignment is > 8 the stack won't fulfill it unless you modify 
```
  /// Add worst-case padding so that future allocations are properly aligned.
  constexpr const uint32_t Alignment = 8;
```
in `openmp/libomptarget/DeviceRTL/src/State.cpp`.
The fact that the state has a fixed alignment right now makes it impossible to allocate higher aligned types anyway.

Proposal:
Add an argument to _alloc_shared that is the alignment as computed above, effecitively making it _alloc_shared_aligned. Modify the stack to actually align the base pointer rather than extend the allocation based on the alignment passed in. Then any type alignment can be handled, including user aligned types.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1475
+                                  CGM.getModule(), OMPRTL___kmpc_free_shared),
+                              {AddrSizePair.first, AddrSizePair.second});
     }
----------------
Not needed. Will cause a warning, no?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:5940
+      } else if (MaybeAlign RetAlign = AI.CB->getRetAlign()) {
+        Alignment = max(Alignment, RetAlign);
       }
----------------
This is sensible but needs a test. You can even do it without the else for all allocations. With the proposed changes above alloc_shared would also fall into the aligned_alloc case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115888