Page MenuHomePhabricator

[FE] Use preferred alignment instead of ABI alignment for complete object when applicable
ClosedPublic

Authored by Xiangling_L on Aug 28 2020, 8:49 AM.

Details

Summary

On some targets, preferred alignment is larger than ABI alignment in some cases. For example, on AIX we have special power alignment rules which would cause that. Previously, to support those cases, we added a “PreferredAlignment” field in the RecordLayout to store the AIX special alignment values in “PreferredAlignment” as the community suggested.

However, that patch alone is not enough. There are places in the Clang where PreferredAlignment
should have been used instead of ABI-specified alignment. This patch is aimed at fixing those
spots.

Diff Detail

Event Timeline

Xiangling_L created this revision.Aug 28 2020, 8:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
Xiangling_L requested review of this revision.Aug 28 2020, 8:49 AM
Xiangling_L retitled this revision from [FE] Use preferred alignment instead of ABI alignment for complete object when applicable Summary: On some targets, preferred alignment is larger than ABI alignment in some cases. For example, on AIX we have special power alignment rules which... to [FE] Use preferred alignment instead of ABI alignment for complete object when applicable .Aug 28 2020, 8:51 AM
Xiangling_L edited the summary of this revision. (Show Details)
martong removed a subscriber: martong.Aug 28 2020, 1:49 PM

Do you have open questions on whether some callsites passing "false" here, should be switched to true? Given what's here, I would say that it definitely does not makes sense to add this parameter everywhere.

So, for getting something committed: please send a new review which makes only the required changes, splitting by logical part of the code. E.g. one change to fix the new/delete alignment, one for the global-variable alignment, and so on if there are others.

clang/lib/CodeGen/CGAtomic.cpp
814 ↗(On Diff #288622)

This is wrong.

clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
252 ↗(On Diff #288622)

Not sure if this is right, since it's looking at individual fields in the struct?

Do you have open questions on whether some callsites passing "false" here, should be switched to true? Given what's here, I would say that it definitely does not makes sense to add this parameter everywhere.

Basically, the places where I changed to true /* NeedsPreferredAlignment */ are ones I'd like reviewers to see if the switch is correct.

clang/lib/CodeGen/CGAtomic.cpp
814 ↗(On Diff #288622)

Can you explain a bit why it's wrong?

Xiangling_L planned changes to this revision.Sep 14 2020, 1:18 PM

Use default argument;

jasonliu added inline comments.Sep 18 2020, 8:36 AM
clang/include/clang/AST/ASTContext.h
259

nit: Extra /// at the beginning of the comment could be removed.

2127

Maybe you want to move up to a newer base? I don't see how this change belong to this patch.

clang/lib/AST/ASTContext.cpp
1872

nit: This comment needs an update, as it could invalidate MemoizedPreferredTypeInfo depending on the flag.

2085

I have two concerns here:

  1. I feel like we are having duplicate logic between here and ASTContext::getPreferredTypeAlign for how to get preferred alignment.
  2. These logic are too AIX-centric and only modified the places for where types are affected by preferred alignment on AIX. What if on some platforms, BuiltinType::Float have different preferred alignments? Or Width is not the preferred alignment on certain platform for double/long double.
clang/lib/CodeGen/TargetInfo.cpp
4538

Question:
It looks like getNaturalAlignIndirect and getTypeAlignInChars here are all returning ABI alignment.
But according to the comments, we should use a preferred alignment when it's a complete object. Isn't this complete object? Or I'm missing something?

jyknight added inline comments.Sep 18 2020, 9:55 AM
clang/include/clang/AST/ASTContext.h
257

Please undo all the changes to the ASTContext API (both in this file and in ASTContext.cpp) which add the boolean NeedsPreferredAlignment to getTypeInfo/getTypeAlign and related functions; it's not necessary. (OK to leave the change to getAlignmentIfKnown).

2154

The change to this function is OK. The wording of the comment change is confusing, though.

How-about:
Return the alignment of a type, in bits, or 0 if the type is incomplete and we cannot determine the alignment (for example, from alignment attributes). The returned alignment is the Preferred alignment if UsePreferredAlignment is true, otherwise is the ABI alignment.

clang/lib/AST/ASTContext.cpp
1849

return NeedsPreferredAlignment ? getPreferredTypeAlign(T) : getTypeAlign(T);

2085

This becomes moot, if my comments are resolved.

2463–2464

Can just call getPreferredTypeAlign here. (Add a QualType overload to getPreferredTypeAlign).

clang/lib/CodeGen/CGAtomic.cpp
814 ↗(On Diff #288622)

It's not allocating new memory, it's accessing a pointer to memory somewhere else, so if alignChars was used, this change would be actively wrong -- using the wrong value.

However, alignChars is unused, so it's wrong only in a way that ultimately makes no difference.

clang/lib/CodeGen/CGExprCXX.cpp
1573

I'd add the 1-line getPreferredTypeAlignInChars function, and call it.

1825

The "getTypeAlignIfKnown" function is so special-purpose, it's probably fine to have this 1 function keep the "Preferred" boolean. This can stay.

clang/lib/CodeGen/ItaniumCXXABI.cpp
2114

getPreferredTypeAlignInChars.

2133

getPreferredTypeAlignInChars.

Xiangling_L added inline comments.Sep 22 2020, 6:45 AM
clang/lib/CodeGen/TargetInfo.cpp
4538

@jyknight Could you shine a light on this? Personally, I would agree that we have complete objects here, so preferred alignment should be used. And if that is true, changes should be applied on all other target within this file?

Xiangling_L marked 16 inline comments as done.

Addressed the comments;

jyknight added inline comments.Sep 23 2020, 4:17 PM
clang/lib/CodeGen/TargetInfo.cpp
4538

This code specifies that aggregate parameters are passed on the stack aligned to CCAlign (4 or 8), even when the aggregate _requires_ higher alignment. E.g. struct X { int a, b; } __attribute__((aligned(8))); would only be passed on the stack with 4-byte alignment for powerpc-aix. The callee needs the object to have the correct 8-byte alignment, so it must copy the parameter to an 8-byte aligned temporary object (this is what "Realign" asks for).

We don't want that overhead for optional (preferred) alignment. It wouldn't be wrong, but would degrade performance and isn't needed. So, no, this shouldn't use preferred alignment.

I'm happy with this now, but please update the commit message to match the updated change.

clang/include/clang/AST/ASTContext.h
2177–2184

Rather than the phrase you added, I'd instead add a new sentence:
"Note: despite the name, the preferred alignment is ABI-impacting, and not an optimization."

efriedma added inline comments.Sep 23 2020, 4:41 PM
clang/lib/CodeGen/TargetInfo.cpp
4538

Are you sure we can get away with skipping the realignment? Consider something like the following:

struct A { double x; };
void f(int x, struct A a) { _Static_assert(_Alignof(a) == 8); }

_Alignof says the alignment is 8, but the IR says "align 4". Is that safe?

jyknight added inline comments.Sep 23 2020, 7:57 PM
clang/lib/CodeGen/TargetInfo.cpp
4538

No. That's not OK, that's definitely a bug -- but not a bug _here_. The bug is that alignof(expression) is returning the wrong answer! What I find interesting is that it doesn't appear to affect codegen -- we seem to correctly generate loads/stores in IR using align 4.

Some more brokenness along the same lines:

clang -fsyntax-only -target powerpc-aix -xc++ - <<EOF
struct A { double x; };
static_assert(alignof(A) == 4, ""); // correct
static_assert(__alignof__(A) == 8, ""); // correct
void f(A* a) {static_assert(alignof(*a) == 4, ""); } // correct
void f(A& a) {static_assert(alignof(a) == 8, ""); } // incorrect
EOF

I note that this is not an AIX-specific bug -- we have the same bug on i386:

clang -fsyntax-only -target i386-linux-gnu -xc++ - <<EOF
static_assert(alignof(double) == 4, "");
static_assert(__alignof__(double) == 8, "");
void f(double* a) {static_assert(alignof(*a) == 4, ""); }
void f(double& a) {static_assert(alignof(a) == 8, ""); }
EOF
Xiangling_L marked 5 inline comments as done.Sep 24 2020, 10:30 AM
Xiangling_L edited the summary of this revision. (Show Details)Sep 24 2020, 10:32 AM

Updated the comments;

jasonliu added inline comments.Mon, Sep 28, 8:10 AM
clang/test/CodeGenCXX/aix-alignment.cpp
17

I believe we would also want to add a test for the delete call to verify the change we made:

void bar() {
  delete[] allocBp();
}
Xiangling_L marked an inline comment as done.

Add delete[] function to the testcase;

jasonliu accepted this revision.Mon, Sep 28, 1:53 PM

LGTM.

This revision is now accepted and ready to land.Mon, Sep 28, 1:53 PM
jyknight accepted this revision.Tue, Sep 29, 4:54 AM