This is an archive of the discontinued LLVM Phabricator instance.

[Annotation] Allows annotation to carry some additional constant arguments.
Needs RevisionPublic

Authored by Tyker on Oct 1 2020, 2:40 AM.

Details

Summary

This allows using annotation in a much more contexts than it currently has.
especially when annotation with template or constexpr.

Diff Detail

Event Timeline

Tyker created this revision.Oct 1 2020, 2:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 1 2020, 2:40 AM
Tyker requested review of this revision.Oct 1 2020, 2:40 AM
aaron.ballman added inline comments.Oct 8 2020, 6:00 AM
clang/lib/CodeGen/CodeGenModule.cpp
2338

Exprs.empty() would be more concise.

2354–2356

You can return the expression directly, which removes a somewhat questionable use of auto.

clang/lib/Sema/SemaDeclAttr.cpp
3683

Just curious, but is the ?: actually necessary? I would have assumed that an empty ArrayRef would return nullptr from data().

3688

auto * since the type is spelled out in the initialization.

Also, I think this is unsafe -- it looks like during template instantiation, any arguments that have a substitution failure will come through as nullptr, and this will crash.

What should happen on template instantiation failure for these arguments? I think the whole attribute should probably be dropped with appropriate diagnostics rather than emitting a partial attribute, but perhaps you have different ideas.

3691

This is unfortunate -- you should be passing in the ParsedAttr object so that the diagnostic engine can take care of properly formatting the diagnostic. For instance, this will incorrectly name the attribute 'annotate' instead of clang::annotate' if that's how the user spelled the attribute. I don't recall whether it will work on an AttributeCommonInfo though.

3694

E->getDependence() smells a bit fishy because this will early bail on any kind of dependency, including an error dependency. Do you mean to care only about type and/or value dependence here?

3704–3705

Under what circumstances would we want the constant expressions to be lvalues? I would have guessed you would want to call Expr::EvaluateAsConstantExpr() instead of either of these.

3730

Hmm, I think you should assert that you never get an identifier at this point -- the parser should be treating all of these as expressions and not simple identifiers.

clang/test/SemaCXX/attr-annotate.cpp
1

Do you mean to emit llvm here? I think that should probably be -fsyntax-only

Tyker updated this revision to Diff 297159.Oct 9 2020, 2:05 AM
Tyker marked 7 inline comments as done.

addressed comments.

Tyker added inline comments.Oct 9 2020, 6:25 AM
clang/lib/Sema/SemaDeclAttr.cpp
3683

maybe it was(in a previous version of the code) but not anymore since there is always at least one argument.

3688

When template instantation fails nullptr is put in the Expr arguments and AddAnnotationAttr will emit a diagnostic and not associate the attribut to the declaration.

3704–3705

Expr::EvaluateAsConstantExpr will evaluate expression in there value category.
this can be quite surprising in some situations like:

const int g_i = 0;
[[clang::annotate("test", g_i)]] void t() {} // annotation carries a pointer/reference on g_i
[[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries the value 0
[[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the value 0

with EvaluateAsRValue in all of the cases above the annotation will carry the value 0.

optionally we could wrap expression with an LValue to RValue cast and call EvaluateAsConstantExpr this should also work.

clang/test/SemaCXX/attr-annotate.cpp
1

The reason i put an -emit-llvm is to also test the asserts in IRgen on more complex code.

(Drive By: This is cool)

(Drive By: This is cool)

I didn't say this before, but yeah, I agree -- this is really quite neat, thank you for working on it!

clang/include/clang/Sema/ParsedAttr.h
1077

as only argument -> as its only argument

1081

The comment can probably be re-flowed so "ambiguity" is on the preceding line?

clang/lib/Sema/SemaDeclAttr.cpp
3688

Great, thank you for the fix!

3693

What is the rationale for bailing out when the constant expression has an APValue result?

3704–3705

Thank you for the explanation. I think wrapping with an lvalue to rvalue conversion may make more sense -- EvaluateAsRValue() tries really hard to get an rvalue out of something even if the standard says that's not okay. It'll automatically apply the lvalue to rvalue conversion if appropriate, but it'll also do more than that (such as evaluating side effects).

clang/test/SemaCXX/attr-annotate.cpp
1

Tests in sema shouldn't typically run codegen, so I'd rather this was -fsyntax-only and test the assertions more explicitly in a codegen-specific test.

Great feature for all the weird pieces of hardware all around the world! :-)

clang/lib/CodeGen/CodeGenFunction.cpp
2229–2233

Curious reindenting with mix-and-match of spaces & tabulations?

clang/lib/CodeGen/CodeGenModule.cpp
2387–2391

Same curious reindenting.

clang/lib/Sema/SemaDeclAttr.cpp
3688

Perhaps a few comments on this clarification would be useful in the code for the plain mortals...

3704–3705

This needs some motivating comments and explanations in the code.
Also the variations on g_i annotation above are quite interesting and should appear in the unit tests. I am unsure they are now.
Perhaps more interesting with other values than 0 in the example. :-)
I guess we can still use [[clang::annotate("test", &g_i)]] void t() {} to have a pointer on g_i and (int&)g_i for reference? To add to the unit tests if not already checked.

Tyker updated this revision to Diff 297409.Oct 10 2020, 5:58 AM
Tyker marked 11 inline comments as done.

addressed comments.

@erichkeane @alexandre.isoard @Naghasan: any feedback in the context of SYCL & HLS C++ about this feature extension?

Tyker added inline comments.Oct 10 2020, 3:18 PM
clang/lib/CodeGen/CodeGenFunction.cpp
2229–2233

these are not tabs i believe this is just how phabricator shows indentation changes.

clang/lib/Sema/SemaDeclAttr.cpp
3693

the intent was that during nested template instantiation arguement will be evaluated as soon as they are neither value nor type dependent and the result will be stored in the ConstantExpr. if an arguement has already been evaluated in a previous instantiation we don't want to evaluate it again.
but because of SubstExpr ConstantExpr are getting removed so it removed it.

3704–3705

i changed to use EvaluateAsConstantExpr + LValueToRvalue cast.

This needs some motivating comments and explanations in the code.
Also the variations on g_i annotation above are quite interesting and should appear in the unit tests. I am unsure they are now.

they are tested not as expressively as above but they are tested.

Perhaps more interesting with other values than 0 in the example. :-)
I guess we can still use [[clang::annotate("test", &g_i)]] void t() {} to have a pointer on g_i

yes this is how it is working.

(int&)g_i for reference? To add to the unit tests if not already checked.

(int&)g_i has the salve value category and same type as g_i it just has a noop cast around it.
pointer an reference are indistinguishable in an APValue the only difference is the c++ type of what they represent.
and they are of course lowered to the same thing. so we only need the pointer case.

keryell added inline comments.Oct 12 2020, 7:13 PM
clang/lib/Sema/SemaDeclAttr.cpp
3704–3705

I see. Thanks for the clarification.

aaron.ballman added inline comments.Oct 19 2020, 6:22 AM
clang/lib/Sema/SemaDeclAttr.cpp
3693

Ah, thank you for the explanation and change.

3696–3697

Should this move up above the point where we're checking whether the expression has an array type?

3706

I'm surprised that the presence of notes alone would mean the attribute argument has an error and should be skipped. Are there circumstances under which Result is true but Notes is non-empty?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
195

No else after a return.

clang/test/SemaCXX/attr-annotate.cpp
97

Some more tests for various constant expression situations that may be weird:

int n = 10;
int vla[n];

[[clang::annotate("vlas are awful", sizeof(vla[++n]))] int i = 0; // reject, the sizeof is not unevaluated

[[clang::annotate("_Generic selection expression should be fine", _Generic(n, int : 0, default : 1))]] int j = 0; // second arg should resolve to 0 fine

void designator();
[[clang::annotate("function designators?", designator)]] int k = 0; // Should work?
Tyker updated this revision to Diff 299333.Oct 20 2020, 4:28 AM
Tyker marked 4 inline comments as done.

addressed comments.

Tyker added inline comments.Oct 20 2020, 4:30 AM
clang/lib/Sema/SemaDeclAttr.cpp
3706

AFAIK
Result means the expression can be folded to a constant.
Note.empty() means the expression is a valid constant expression in the current language mode.

the difference is observable in code like:

constexpr int foldable_but_invalid() {
  int *A = new int(0);
  return *A;
}

[[clang::annotate("", foldable_but_invalid())]] void f1() {}

foldable_but_invalid retruns a constant but any constant evaluation of this function is invalid because it doesn't desallocate A.
with !Notes.empty() this fails, without it no errors occurs.

i think it is desirable that attributes don't diverge from the language mode.
I added a test for this.

clang/test/SemaCXX/attr-annotate.cpp
97

added CK_FunctionToPointerDecay cast when needed
the rest was working as is.

aaron.ballman accepted this revision.Oct 22 2020, 6:15 AM

LGTM aside from a request for a comment to be added. Thank you!

clang/lib/Sema/SemaDeclAttr.cpp
3706

Ah, thank you for explaining that! Can you add a comment to the code to make that clear for others who may run across it?

This revision is now accepted and ready to land.Oct 22 2020, 6:15 AM
Tyker added a comment.Oct 22 2020, 9:43 AM

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

He means this review comment he made requesting a comment in the code: >Ah, thank you for explaining that! Can you add a comment to the code to make that clear for others who may run across it?

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

Oh gosh no! I just meant I was asking for a comment to be added in SemaDeclAttr.td before you commit. Sorry for the confusion! :-)

Tyker updated this revision to Diff 300213.Oct 23 2020, 3:37 AM

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

Oh gosh no! I just meant I was asking for a comment to be added in SemaDeclAttr.td before you commit. Sorry for the confusion! :-)

Thank for the review.

This revision was landed with ongoing or failed builds.Oct 26 2020, 2:50 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 26 2020, 3:49 AM

Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt

Please take a look, and revert for now if it takes a while to fix.

This also causes the AnnotateFunctions example to no longer build:

/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp: In member function ‘virtual bool {anonymous}::AnnotateFunctionsConsumer::HandleTopLevelDecl(clang::DeclGroupRef)’:
/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:36:70: error: no matching function for call to ‘clang::AnnotateAttr::CreateImplicit(clang::ASTContext&, const char [19])’
                                                  "example_annotation"));
                                                                      ^

it looks like it needs to be adjusted for the extra variadic arg that the attribute now has.

Tyker added a comment.Oct 27 2020, 2:30 AM

Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt

Please take a look, and revert for now if it takes a while to fix.

this is fixed by 4afa077899b

This also causes the AnnotateFunctions example to no longer build:

/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp: In member function ‘virtual bool {anonymous}::AnnotateFunctionsConsumer::HandleTopLevelDecl(clang::DeclGroupRef)’:
/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:36:70: error: no matching function for call to ‘clang::AnnotateAttr::CreateImplicit(clang::ASTContext&, const char [19])’
                                                  "example_annotation"));
                                                                      ^

it looks like it needs to be adjusted for the extra variadic arg that the attribute now has.

this is fixed by 2618247c61c

vhscampos added a subscriber: vhscampos.EditedNov 6 2020, 7:34 AM

@Tyker This is causing another build failure in another example:

[2858/4034] Building CXX object tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o
/Attribute.cpp:73:16: error: no matching function for call to 'Create'
    D->addAttr(AnnotateAttr::Create(S.Context, "example(" + Str.str() + ")",
               ^~~~~~~~~~~~~~~~~~~~
tools/clang/include/clang/AST/Attrs.inc:885:24: note: candidate function not viable: requires at least 4 arguments, but 3 were provided
  static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, Expr * *Args, unsigned ArgsSize, const AttributeCommonInfo &CommonInfo = {SourceRange{}});
                       ^
tools/clang/include/clang/AST/Attrs.inc:887:24: note: candidate function not viable: requires 6 arguments, but 3 were provided
  static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, Expr * *Args, unsigned ArgsSize, SourceRange Range, AttributeCommonInfo::Syntax Syntax);
                       ^
1 error generated.

Please build with -DCLANG_BUILD_EXAMPLES=ON to check if all bugs are gone.

jdoerfert reopened this revision.Feb 9 2021, 11:22 AM

The lang ref needs to reflect the new type https://llvm.org/docs/LangRef.html#llvm-var-annotation-intrinsic
Please also review the auto-upgrade patch for this: https://reviews.llvm.org/D95993

This revision is now accepted and ready to land.Feb 9 2021, 11:22 AM
jdoerfert requested changes to this revision.Feb 9 2021, 11:23 AM

status change, see last message

This revision now requires changes to proceed.Feb 9 2021, 11:23 AM
Tyker added a comment.Feb 13 2021, 4:29 AM

I added the changes to the langref in https://reviews.llvm.org/D96646