Page MenuHomePhabricator

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

Authored by Tyker on Thu, Oct 1, 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.Thu, Oct 1, 2:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptThu, Oct 1, 2:40 AM
Tyker requested review of this revision.Thu, Oct 1, 2:40 AM
aaron.ballman added inline comments.Thu, Oct 8, 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
3688

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

3693

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.

3696

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.

3699

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?

3709–3710

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.

3739

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
2

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

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

addressed comments.

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

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

3693

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.

3709–3710

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
2

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
3693

Great, thank you for the fix!

3698

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

3709–3710

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
2

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
2259–2263

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

clang/lib/CodeGen/CodeGenModule.cpp
2386–2390

Same curious reindenting.

clang/lib/Sema/SemaDeclAttr.cpp
3693

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

3709–3710

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.Sat, Oct 10, 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.Sat, Oct 10, 3:18 PM
clang/lib/CodeGen/CodeGenFunction.cpp
2259–2263

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

clang/lib/Sema/SemaDeclAttr.cpp
3698

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.

3709–3710

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.Mon, Oct 12, 7:13 PM
clang/lib/Sema/SemaDeclAttr.cpp
3709–3710

I see. Thanks for the clarification.

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

Ah, thank you for the explanation and change.

3701–3702

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

3711

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.Tue, Oct 20, 4:28 AM
Tyker marked 4 inline comments as done.

addressed comments.

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

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.