This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][FIX] Do not overalign mapped structures
ClosedPublic

Authored by jdoerfert on Jan 25 2023, 3:01 PM.

Details

Summary

While we potentially need to align partially mapped structs more than
the first member, we do not need to align past the struct itself. This
prevents us from moving the base pointer past the struct beginning too.

See https://reviews.llvm.org/D142508 for a discussion.

Diff Detail

Event Timeline

jdoerfert created this revision.Jan 25 2023, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 3:01 PM
jdoerfert requested review of this revision.Jan 25 2023, 3:01 PM

Argh, this is not right, it's not what I proposed in the other patch. Will update it later.

jdoerfert updated this revision to Diff 492299.Jan 25 2023, 5:15 PM

Passes the tests now.

jdoerfert updated this revision to Diff 492303.Jan 25 2023, 5:57 PM

Add second test scenario.

jhuber6 accepted this revision.Jan 25 2023, 6:05 PM
jhuber6 added inline comments.
openmp/libomptarget/src/omptarget.cpp
113

Should try to be consistent with C++ style casts.

This revision is now accepted and ready to land.Jan 25 2023, 6:05 PM
grokos added inline comments.Jan 25 2023, 11:27 PM
openmp/libomptarget/src/omptarget.cpp
78–79

Can you fix this comment as well? Alignment is not necessarily 8 anymore, it could be up to 16.

grokos accepted this revision.Jan 25 2023, 11:29 PM

Thank you for the patch! I checked it with the D135462 PR that changes alloca alignment - everything works fine.

pavelkopyl added inline comments.Jan 26 2023, 11:17 AM
openmp/libomptarget/test/mapping/low_alignment.c
10

The only thing is that alignment of the structure 's' may vary depending on many factors. For example, on x64 it's likely 8-aligned. I mean here we check not lowest possible alignment, but rather arbitrary one. Is that OK?

pavelkopyl accepted this revision.Jan 26 2023, 11:19 AM
jdoerfert added inline comments.Feb 3 2023, 2:16 AM
openmp/libomptarget/test/mapping/low_alignment.c
10

The struct (should be) aligned properly for the member that requires the largest alignment. Which is exactly what we are looking for because we might represent only part of the struct and that part might contain this member.

jhuber6 updated this revision to Diff 494593.Feb 3 2023, 5:33 AM

Addressing comments

Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 5:33 AM
This revision was automatically updated to reflect the committed changes.