[PATCH] Support the assume_aligned function attribute

[Richard Smith]

================
Comment at: lib/CodeGen/CGExpr.cpp:3379-3382
@@ -3378,2 +3378,6 @@
 
-  return EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+  RValue Ret = EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+
+  if (Ret.isScalar() && TargetDecl) {
+    if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
+      llvm::Value *OffsetValue = nullptr;
----------------
hfinkel wrote:
> rsmith wrote:
> > Is this really the right place for this code, rather than in the called `EmitCall` function? Various places call the lower-level overload directly.
> 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?
`EmitCXXMemberOrOperatorCall`, `EmitCXXMemberPointerCallExpr`, and `EmitNewDeleteCall` should also get this handling.

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

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1130
@@ -1129,2 +1129,3 @@
+bool Sema::isValidPointerAttrType(QualType T) {
   T = T.getNonReferenceType();
 
----------------
hfinkel wrote:
> rsmith wrote:
> > 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".)
> 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.
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!)

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:203-205
@@ +202,5 @@
+    const AssumeAlignedAttr *AssumeAligned = dyn_cast<AssumeAlignedAttr>(TmplAttr);
+    if (AssumeAligned && (AssumeAligned->getAlignment()->isValueDependent() ||
+                          (AssumeAligned->getOffset() &&
+                           AssumeAligned->getOffset()->isValueDependent()))) {
+      instantiateDependentAssumeAlignedAttr(*this, TemplateArgs, AssumeAligned, New);
----------------
hfinkel wrote:
> rsmith wrote:
> > 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.
> 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.
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>();

http://reviews.llvm.org/D4601