[PATCH] Support the assume_aligned function attribute

[Aaron Ballman]

On Mon, Jul 21, 2014 at 12:36 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Aaron Ballman" <aaron.ballman at gmail.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "Richard Smith" <richard at metafoo.co.uk>, "llvm cfe" <cfe-commits at cs.uiuc.edu>,
>> reviews+D4601+public+521bc0d2736e59d3 at reviews.llvm.org
>> Sent: Monday, July 21, 2014 11:33:17 AM
>> Subject: Re: [PATCH] Support the assume_aligned function attribute
>>
>> On Mon, Jul 21, 2014 at 12:25 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> > ----- Original Message -----
>> >> From: "Aaron Ballman" <aaron.ballman at gmail.com>
>> >> To: reviews+D4601+public+521bc0d2736e59d3 at reviews.llvm.org
>> >> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Richard Smith"
>> >> <richard at metafoo.co.uk>, "llvm cfe" <cfe-commits at cs.uiuc.edu>
>> >> Sent: Monday, July 21, 2014 11:22:01 AM
>> >> Subject: Re: [PATCH] Support the assume_aligned function attribute
>> >>
>> >> On Sun, Jul 20, 2014 at 7:25 PM, Richard Smith
>> >> <richard at metafoo.co.uk> wrote:
>> >> > Somewhat unrelated to this patch in particular, but we should
>> >> > add a
>> >> > UBSan check to catch failed assumptions.
>> >> >
>> >> > ================
>> >> > Comment at: include/clang/Basic/Attr.td:867
>> >> > @@ +866,3 @@
>> >> > +                             "ExpectedFunctionOrMethod">;
>> >> > +  let Args = [IntArgument<"Alignment">,
>> >> > DefaultIntArgument<"Offset", 0>];
>> >> > +  let Documentation = [Undocumented];
>> >> > ----------------
>> >> > These should probably both be `ExprArgument`s, in order to
>> >> > support
>> >> > dependent alignment/offset expressions in templates.
>> >> >
>> >> > ================
>> >> > Comment at: include/clang/Basic/Attr.td:868
>> >> > @@ +867,3 @@
>> >> > +  let Args = [IntArgument<"Alignment">,
>> >> > DefaultIntArgument<"Offset", 0>];
>> >> > +  let Documentation = [Undocumented];
>> >> > +}
>> >> > ----------------
>> >> > Please add documentation.
>> >> >
>> >> > ================
>> >> > Comment at: lib/CodeGen/CGExpr.cpp:3352
>> >> > @@ +3351,3 @@
>> >> > +
>> >> > +  if (Ret.isScalar() && TargetDecl)
>> >> > +    if (const AssumeAlignedAttr *AA =
>> >> > ----------------
>> >> > Please add parens around this `if`.
>> >> >
>> >> > ================
>> >> > Comment at: lib/Sema/SemaDeclAttr.cpp:1219-1227
>> >> > @@ +1218,11 @@
>> >> > +
>> >> > +  if (Attr.getNumArgs() > 2) {
>> >> > +    S.Diag(Attr.getLoc(),
>> >> > diag::err_attribute_too_many_arguments)
>> >> > +      << Attr.getName() << 2;
>> >> > +    return;
>> >> > +  } else if (Attr.getNumArgs() < 1) {
>> >> > +    S.Diag(Attr.getLoc(),
>> >> > diag::err_attribute_too_few_arguments)
>> >> > +      << Attr.getName() << 1;
>> >> > +    return;
>> >> > +  }
>> >> > +
>> >> > ----------------
>> >> > I think there's a helper function for this sequence of
>> >> > checks/diagnostics. If not, there should be.
>> >>
>> >> They aren't required -- the common handler takes care of that
>> >> stuff.
>> >
>> > No, I put them in specifically because the common handler did not.
>> > I noticed because when I was writing the regression tests, if I
>> > passed too few parameters (specifically none), the later call to
>> > getArg would assert.
>>
>> Ahhhh shoot, the optional argument is why that's needed (see
>> handleCommonAttributeFeatures()). Sorry for leading you astray there;
>> I forgot that I hadn't gotten around to implementing optional
>> argument
>> handlers there yet (which is where this logic really should live).
>
> Where is that? If you don't get to it first, I'll just implement the correct logic there.

SemaDeclAttr.cpp, about line 4007. We have:

// If there are no optional arguments, then checking for the argument count
  // is trivial.
  if (Attr.getMinArgs() == Attr.getMaxArgs() &&
      !checkAttributeNumArgs(S, Attr, Attr.getMinArgs()))
    return true;

This needs to be updated for the else case where minArgs and maxArgs
don't match up. Then look for any place using those "incorrect number
of args" cases in SemaDeclAttr.cpp and yank them out (I don't believe
there are a lot of them). If you don't get to it, I can tackle it at
some point as well.

Thanks!

~Aaron