This is an archive of the discontinued LLVM Phabricator instance.

Support the assume_aligned function attribute
ClosedPublic

Authored by hfinkel on Jul 20 2014, 2:49 PM.

Details

Summary

In addition to __builtin_assume_aligned, GCC also supports an assume_aligned attribute which specifies the alignment (and optional offset) of the function's return value. Building on the llvm.assume patches, and also functionality added in http://reviews.llvm.org/D149, this patch implements support for the assume_aligned attribute in Clang.

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 11704.Jul 20 2014, 2:49 PM
hfinkel retitled this revision from to Support the assume_aligned function attribute.
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added reviewers: rsmith, aaron.ballman.
hfinkel added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Jul 20 2014, 4:25 PM

Somewhat unrelated to this patch in particular, but we should add a UBSan check to catch failed assumptions.

include/clang/Basic/Attr.td
871

These should probably both be ExprArguments, in order to support dependent alignment/offset expressions in templates.

872

Please add documentation.

lib/CodeGen/CGExpr.cpp
3381

Please add parens around this if.

lib/Sema/SemaDeclAttr.cpp
1248–1256

I think there's a helper function for this sequence of checks/diagnostics. If not, there should be.

1261–1264

Do you really need to check that the Offset fits into an uint32_t? Truncating the offset to the width of Alignment seems correct in all cases.

rsmith added inline comments.Jul 20 2014, 4:26 PM
lib/CodeGen/CGExpr.cpp
3381

s/parens/braces/ =)

hfinkel updated this revision to Diff 13228.Sep 3 2014, 2:58 PM
hfinkel edited edge metadata.

Updated per review comments from Richard and Aaron (and rebased).

rsmith added inline comments.Sep 3 2014, 5:09 PM
lib/CodeGen/CGExpr.cpp
3379–3382

Is this really the right place for this code, rather than in the called EmitCall function? Various places call the lower-level overload directly.

lib/Sema/SemaDeclAttr.cpp
1130

Is this really appropriate for your attribute? (It makes almost no sense for __attribute__((nonnull)) either, but I couldn't find anyone at Apple to tell me what the relevant radar says, so I have no idea why we have this "feature".)

1132–1133

Update this comment, please.

1263

You don't need the type-dependency check here.

1285

Likewise here.

lib/Sema/SemaTemplateInstantiateDecl.cpp
203–205

You shouldn't have these 'is dependent' checks here; the other code above and below is wrong to include them. Testcase:

template<typename T> __attribute__((assume_aligned(sizeof(int(T()))))) T *f();
void *p = f<void>();

This should result in a hard error, because int(void()) is invalid, but I think you'll accept it, because sizeof(int(T())) is not value-dependent, so you never actually perform the substitution.

hfinkel added inline comments.Sep 5 2014, 1:26 PM
lib/CodeGen/CGExpr.cpp
3379–3382

When I first looked, I thought that there were no other relevant callers, but maybe EmitCallAndReturnForThunk and EmitForwardingCallToLambda would need this handling too?

Because of the way that the lower-level EmitCall is structured, I'd need to wrap it (it has too many early returns). What would I call it?

lib/Sema/SemaDeclAttr.cpp
1130

Nice :-) -- Honestly, I don't have a strong opinion regarding whether or not we strip references, but I like the idea of consistency between the two attributes. The set of returned pointers about which I can assert something about the alignment should be the same as that about which I can assert a nonnull property.

That having been said, it looks like the CodeGen for returns_nonnull that adds the nonnull IR attribute (and the associated UBSan code) does not handle references-to-pointers correctly. I'll fix that as a follow-up too.

lib/Sema/SemaTemplateInstantiateDecl.cpp
203–205

Interestingly, the current code does generate an error:

invalid application of 'sizeof' to a function type

Removing the type-dependent checks does not seem to change any of the current behavior.

rsmith added inline comments.Sep 5 2014, 2:08 PM
lib/CodeGen/CGExpr.cpp
3379–3382

EmitCXXMemberOrOperatorCall, EmitCXXMemberPointerCallExpr, and EmitNewDeleteCall should also get this handling.

Maybe factor out the switch that produces the returned RValue from the call?

lib/Sema/SemaDeclAttr.cpp
1130

It seems really weird for either of these to look through references to the referenced pointer. And it's ambiguous: does the attribute apply to the reference or to the pointer that the reference refers to? (For the nonnull attribute, GCC and Clang make different choices here, at least in the cases where you can persuade GCC not to reject the attribute!)

lib/Sema/SemaTemplateInstantiateDecl.cpp
203–205

Ha, sorry, bitten by a parse ambiguity. Six closing parens wasn't enough; let's go up to seven:

template<typename T> __attribute__((assume_aligned(sizeof((int(T())))))) T *f();
void *p = f<void>();
hfinkel added inline comments.Sep 5 2014, 11:30 PM
lib/Sema/SemaDeclAttr.cpp
1130

Indeed, unlike nonnull, this does become ambiguous. Perhaps it is best to prohibit it?

How do Clang and GCC make different choices for nonnull; is that a bug? Because a reference cannot be null, it would seem that it should apply to the pointer being bound.

rsmith added inline comments.Sep 6 2014, 9:01 PM
lib/Sema/SemaDeclAttr.cpp
1130

Here's what I can piece together:

  • GCC does not intend to support nonnull on reference parameters. It rejects nonnull(N) where parameter N is a reference, *but* if you apply nonnull with no N, it applies to both pointer and reference parameters, and happens to have the semantics of checking for "null references".
  • Clang made up its own thing in response to some non-publicly-visible radar issue, where it treats the nonnull attribute on references-to-pointers as applying to the pointer within the reference. This makes little sense to me, but there it is. We naturally can't optimize on the basis of nonnull on a reference-to-pointer like we can on a real pointer parameter/return value (we have no IR representation for that).

It seems our options are to either not support assume_aligned on references (which would be a shame, because it's natural and useful there just as it is for pointers), or make nonnull and assume_aligned inconsistent on pointers-to-references, or remove our (dubious, IMO) extension of the GNU nonnull semantics. I'm not happy with the third option since I don't know what the motivation for the extension is nor how many people might be relying on it, but either of the first two seem fine to me. I don't think the connection between nonnull and assume_aligned is strong enough that the difference would be jarring for people.

hfinkel added inline comments.Sep 8 2014, 5:06 AM
lib/CodeGen/CGExpr.cpp
3379–3382

I tried using a lambda, let me know what you think.

lib/Sema/SemaDeclAttr.cpp
1130

Alright, I'll go with option 1, we'll have it apply to references in the natural way.

lib/Sema/SemaTemplateInstantiateDecl.cpp
203–205

Test case added.

hfinkel updated this revision to Diff 13386.Sep 8 2014, 5:08 AM

Updated per Richard's review.

rsmith accepted this revision.Sep 25 2014, 4:57 PM
rsmith edited edge metadata.
This revision is now accepted and ready to land.Sep 25 2014, 4:57 PM
hfinkel closed this revision.Sep 25 2014, 10:23 PM

r218500, thanks!