[Openmp-commits] [PATCH] D142508: [OpenMP][libomptarget] Fix alignment calculation for mapping struct members.

Tue Jan 24 17:44:01 PST 2023

jdoerfert added a comment.

In D142508#4078655 <https://reviews.llvm.org/D142508#4078655>, @asavonic wrote:

> Just to summarize, the current implementation of padding in libomptarget has a bug, but it was hidden until we changed default alignment of allocas in D135462 <https://reviews.llvm.org/D135462>. This bug is now gating D135462 <https://reviews.llvm.org/D135462> and Pavel and I were trying to investigate it.

Yes, and unfortunately yes.

In D142508#4078640 <https://reviews.llvm.org/D142508#4078640>, @grokos wrote:

> In D142508#4078632 <https://reviews.llvm.org/D142508#4078632>, @jdoerfert wrote:
>
>> The struct will have an alignment of 8 then, it will adjust according to members.
>
> Sure, but that's true for the entire struct on the host. If you map it partially on the device, you may break this alignment, i.e. the partially mapped struct may no longer satisfy alignment requirements.

The base has always the alignment we are looking for. And "base" here is the entire struct (aka `HstPtrBase`). 
Right now we assume the base is at least 8 aligned, but in the case of two integers it's not. Hence the error.
We cannot align any member with more alignment as the base, which is what is happening here.
However, clang will align the base according to the members, so if there is an 8byte pointer (or double) in there, base will be 8 aligned.
If there is a long double in there, base will be 16 aligned, etc.

All that said, if you think my reasoning is flawed, could you provide an example?
My development server is down so I can't write the patch I have in mind myself rn.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142508