[PATCH] Support the assume_aligned function attribute

[Hal Finkel]

----- 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: Thursday, July 31, 2014 11:50:59 AM
> Subject: Re: [PATCH] Support the assume_aligned function attribute
> 
> 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.
> 
> Just to circle back around on this, I got around to implementing this
> today in r214407. So *now* you should be able to elide the manual arg
> count checking with this patch.

Great, thanks!

 -Hal

> 
> ~Aaron
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory