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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

I will split this into two revisions, one handling the return alignment attribute in the Attributor, and one adding alignment information to the `__kmpc_alloc_shared` OpenMP runtime call, turning it into an aligned allocation.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1411
+        CGM.getLLVMContext(), llvm::Attribute::Alignment,
+        CGM.getContext().getTypeAlignInChars(VarTy).getQuantity()));
 
----------------
jdoerfert wrote:
> 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.
That was an original though, I was hoping to avoid the extra work, but I think this is definitely the only way to solve this reasonably, it might also allow us to use the stack more efficiently. We'll still want this alignment information, but we'll need to inform the runtime of the expected alignment.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1475
+                                  CGM.getModule(), OMPRTL___kmpc_free_shared),
+                              {AddrSizePair.first, AddrSizePair.second});
     }
----------------
jdoerfert wrote:
> Not needed. Will cause a warning, no?
Forgot about this, not intended to be included.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:5940
+      } else if (MaybeAlign RetAlign = AI.CB->getRetAlign()) {
+        Alignment = max(Alignment, RetAlign);
       }
----------------
jdoerfert wrote:
> 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.
Yes, we want this regardless because all `malloc` like calls now seem to have alignment attributes, which makes sure we respect the alignment of the original malloc call. I can probably split this into another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115888