[PATCH] Support the assume_aligned function attribute

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Aaron Ballman" <aaron.ballman at gmail.com>
> Cc: "Richard Smith" <richard at metafoo.co.uk>, reviews+D4601+public+521bc0d2736e59d3 at reviews.llvm.org, "llvm cfe"
> <cfe-commits at cs.uiuc.edu>
> Sent: Monday, July 21, 2014 11:25:14 AM
> Subject: Re: [PATCH] Support the assume_aligned function attribute
> 
> ----- 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.

If you want a quick example of this behavior (without this patch), try this:

$ cat /tmp/f.cu 
void func(int a, int b) __attribute__((launch_bounds()));

$ clang -cc1 -fsyntax-only -verify /tmp/f.cu

and you get this error:

File /tmp/f.cu Line 1: 'launch_bounds' attribute takes no more than 2 arguments

which is wrong because it is hitting the code inside handleLaunchBoundsAttr (where there is a FIXME to provide the correct error in this case).

 -Hal

> 
>  -Hal
> 
> > 
> > >
> > > ================
> > > Comment at: lib/Sema/SemaDeclAttr.cpp:1232-1235
> > > @@ +1231,6 @@
> > > +    return;
> > > +  if (Attr.getNumArgs() > 1 && !checkUInt32Argument(S, Attr,
> > > +
> > >                                                    Attr.getArgAsExpr(1),
> > > +                                                    Offset, 2))
> > > +    return;
> > > +
> > > ----------------
> > > 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.
> > 
> > This doesn't check that it fits into a uint32_t. It checks that
> > it's
> > a
> > valid integer constant expression and errs if it's not. It then
> > gets
> > you the value as a uint32_t.
> > 
> > ~Aaron
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

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