Page MenuHomePhabricator

[AIX] Implement AIX special alignment rule about double/long double
Changes PlannedPublic

Authored by Xiangling_L on Mon, May 11, 9:06 AM.

Details

Summary

Implement AIX special alignment rule by recursively checking if the
'real' first member is a double/long double. If yes, then this member
should be naturally aligned to 8 rather than 4.

Notes:

  1. ASTRecordLayout Layout.Alignment records the min alignment of object, which is also the value _Alignof(x) should return;

2.Layout.NonVirtualAlignment records min alignment of nv part of object

  1. The size/nvsize of object will be rounded up by recursively checking if the first member of object is double/long double.

4.getPreferredTypeAlign = __alignof(x) = preferred alignment of object is the real alignment in AIX's semantic.

  1. getPreferredTypeAlign return true alignment by recursively checking if the first member of object is double/long double.

Diff Detail

Event Timeline

Xiangling_L created this revision.Mon, May 11, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 11, 9:07 AM
Xiangling_L edited the summary of this revision. (Show Details)Mon, May 11, 9:13 AM
Xiangling_L edited the summary of this revision. (Show Details)Mon, May 11, 9:16 AM

This approach seems to reflect the consensus from the mailing list.

clang/include/clang/AST/RecordLayout.h
74

If we have to keep around extra data anyway, probably better to add a field for the preferred alignment, so we can keep the preferred alignment handling together.

clang/lib/AST/ASTContext.cpp
2518

We try to avoid OS-specific checks here. But I'm not sure what this should look like on other targets.

2527

I'd prefer to centralize the check for "AIX struct layout" in one place, as opposed to putting checks for AIX targets in multiple places.

clang/lib/AST/RecordLayoutBuilder.cpp
3438

Please don't make the dumping code lie about the "align". If you want to dump the preferred alignment in addition, that would be fine.

jasonliu added inline comments.Wed, May 20, 10:49 AM
clang/include/clang/AST/RecordLayout.h
74

We could get the MaxFieldAlignment in the same way ItaniumRecordLayoutBuilder::InitializeLayout get. Do we need to keep an extra data?

Xiangling_L planned changes to this revision.Wed, May 20, 12:41 PM
jasonliu added inline comments.Wed, May 20, 1:08 PM
clang/lib/AST/ASTContext.cpp
2533

I guess another thing that worth considering is how we support "pragma align(nature)" in the future. It requires us to get a normal alignment for AIX which means double will be aligned 8 byte in the structure.
Right now, we always set the double to be aligned 4 byte in PPC.h, but it's not necessary the case if that pragma is in effect. How double should be aligned in a structure is really position dependent (is the structure comes after pragma align(nature) or pragma align(power)?) I'm not very clear on how we would handle that with the current architect.

efriedma added inline comments.Wed, May 20, 2:08 PM
clang/include/clang/AST/RecordLayout.h
74

The idea here is that we want to make the division of work more clear: RecordLayoutBuilder actually does the relevant computation, and ASTContext just returns the result.

clang/lib/AST/ASTContext.cpp
2533

We convert pragmas like that into attributes in the AST. See Sema::AddAlignmentAttributesForRecord.

In the commit message, you refer to the preferred alignemtn as the "true" alignment, but that's misleading. As discussed on the mailing list thread, it's not true alignment at all.

clang/include/clang/AST/RecordLayout.h
74

+1 to just storing the preferred alignment on the RecordLayout -- it already had to compute it to compute the size-adjustment for AIX, anyhow. Then you no longer need to store MaxFieldAlignment.

clang/lib/AST/ASTContext.cpp
2506

I think from here on down is currently X86-specific, even though it's not phrased that way right now.

It only applies if this target hook doesn't disable it, and if alignof(X) < sizeof(X), for X in {double, long long, unsigned long long}. It would be good to try to determine if there's any other platforms for which those conditions actually exist today, other than x86-32. And then determine if this code block actually _should_ trigger there. (I suspect not.) Then, mark this stuff as truthfully completely-target-specific, instead of pretending otherwise, as we do now.

clang/lib/AST/RecordLayoutBuilder.cpp
1944

name it RoundSizeTo, to indicate that this is to be used _only_ for aligning the size, and isn't actually the alignment.