Page MenuHomePhabricator

Add nomerge statement attribute to clang
ClosedPublic

Authored by zequanwu on Apr 29 2020, 1:12 PM.

Details

Reviewers
rnk
aaron.ballman
Summary

Add nomerge statement attribute which applies to all call expressions inside the statement.
Related to D78659

Diff Detail

Event Timeline

zequanwu created this revision.Apr 29 2020, 1:12 PM
rnk added inline comments.Apr 29 2020, 2:36 PM
clang/include/clang/Basic/Attr.td
1804

Clang supports many function-like things that are not functions, for example, Objective C method declarations are not considered FunctionDecls in the AST. Blocks are another function-like thing. I see below there is a FunctionLike subject type. Lets use that here.

I would like to make it possible to apply this attribute directly to call expressions as well, but I don't see a good example of how to do that in the existing attribute. I'd like to be able to do this, for example:

void __attribute__((noreturn)) bar();
void foo(bool cond) {
  if (cond)
    [[clang::nomerge]] bar();
  else
    [[clang::nomerge]] bar();
}

This gives the user fine-grained control. It allows them to call some existing API, like perhaps RaiseException, of which they do not control the declaration, and mark the calls as unmergeable.

clang/test/CodeGen/attr-nomerge.c
15 ↗(On Diff #261001)

These numbers can be unstable. Generally the best practice is to do something like this:

// CHECK: declare void @bar() #[[NOMERGEATTR:\d+]]
// CHECK: attributes #[[NOMERGEATTR]] = { nomerge {{.*}} }

This makes the test more readable and more resilient in the case that the attribute set numbers change.

zequanwu updated this revision to Diff 261292.Apr 30 2020, 11:06 AM
zequanwu marked 2 inline comments as done.Apr 30 2020, 11:09 AM
zequanwu added inline comments.
clang/include/clang/Basic/Attr.td
1804

It seems like there is no exiting example of call site attributes. https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CGCall.cpp#L4767. And the function EmitCall may not know existence of call site attributes.

aaron.ballman added a subscriber: aaron.ballman.

This should also have Sema tests ensuring that it only applies to the correct subjects, accepts no arguments, etc.

clang/include/clang/Basic/Attr.td
1804

I would like to make it possible to apply this attribute directly to call expressions as well, but I don't see a good example of how to do that in the existing attribute.

You would have to use a statement attribute rather than a declaration attribute for this, and we have a few of those but none that work on call expressions specifically.

One question I have about that idea is whether attributes give you fine enough levels of control (I don't know enough about nomerge to answer this myself). e.g., foo(bar(), bar()); // Do you want to nomerge the bar() calls, the foo() call, or all three? (You can only add the attribute to the statement expression, not to individual expressions.)

rnk added inline comments.Apr 30 2020, 2:12 PM
clang/include/clang/Basic/Attr.td
1804

I guess what I really wanted was an expression attribute, so you can put it on the calls to bar. :)

But given that that doesn't exist, I think it would be OK to do either one of the following:

  • Apply nomerge attribute to all call sites in the whole Stmt
  • Apply nomerge only to the first CallExpr found inside the Stmt

I think the simplest implementation is to, in CodeGenFunction, set a flag (NoMerge) on CodeGenFunction when emitting such an attributed statement with this attribute. Then set it back on exit. Something like that.

rnk added inline comments.Apr 30 2020, 2:13 PM
clang/include/clang/Basic/Attr.td
1804

It's worth considering some test cases where the call expression is not the top-most AST node in the expression:

[[clang::nomerge]] (x = 42, abort());
[[clang::nomerge]] (void)(throwException());

We can probably make an ExprWithTemporaries wrapper, ParenExpr wrapper, or others.

zequanwu updated this revision to Diff 261406.Apr 30 2020, 5:34 PM

change nomerge to statement attribute

This is missing Sema tests ensuring that the attribute only appertains to the correct subject, does not accept arguments, etc.

clang/include/clang/Basic/AttrDocs.td
356

Looks like the docs need updating still. The functions aren't marked nomerge, statements are. This should explain the behavior of how we are lowering the nomerge attribute on call sites.

clang/lib/CodeGen/CGCall.cpp
4825

Missing a full stop at the end of the comment.

clang/lib/CodeGen/CGStmt.cpp
612

Please don't name the declaration Attr as that's a type name.

clang/lib/Sema/SemaStmtAttr.cpp
176

Should there be semantic checking here? For instance, I would expect that writing a nomerge attribute on a statement which contains no call expressions should be diagnosed because something unexpected has happened.

zequanwu updated this revision to Diff 261698.May 3 2020, 9:26 AM
zequanwu marked 4 inline comments as done.

Add Sema test.
Check if nomerge attribute has no arguments and statement contains call expressions.

aaron.ballman added inline comments.May 4 2020, 11:22 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2766–2767

I think this should be in the IgnoredAttributes group as well.

clang/lib/Sema/SemaStmtAttr.cpp
174

You'll need to check that S is nonnull, children() can return null children in some cases. Also, S should be a const Stmt* and I wouldn't mind a newline before the function definition.

176–180

I think you could replace this with: llvm::any_of(S->children(), hasCallExpr);

185

attribute doesn't meet the usual naming conventions. Can probably just go with NMA or something lame like that.

190

You should be able to just pass A directly here instead.

clang/test/CodeGen/attr-nomerge.cpp
7

I'd appreciate seeing some more complex tests showing the behavior of the attribute. As some examples:

// Does this work on dynamically initialized global statics?
[[clang::nomerge]] static int i = foo() + bar();

void func() {
  // Is the lambda function call operator nomerge, the contained calls within the lambda, or none of the above?
  [[clang::nomerge]] [] { foo(); bar(); }(); 

  // Do we mark the implicit function calls to begin()/end() as nomerge?
  [[clang::nomerge]] for (auto foo : some_container) {}

  // Does it apply to builtins as well?
  [[clang::nomerge]] foo(__builtin_strlen("bar"), __builtin_strlen("bar"));

  // Does it apply to the substatement of a label?
  [[clang::nomerge]] how_about_this: f(bar(), bar());

  // Does it apply across the components of a for statement?
  for (foo(); bar(); baz()) {}
}
rnk added inline comments.May 4 2020, 2:32 PM
clang/lib/CodeGen/CGStmt.cpp
612

Can we use S.hasAttr, or does that not exist for statements?

616

Please use SaveAndRestore, since this could be re-entrant. Consider cases like:

[[clang::nomerge]] foo(1, ({bar(); [[clang::nomerge]] baz(); 2}), qux());

The inner nomerge will "unset" the outer one too early.

This is similar to what will happen if we allow nomerge on compound statements as in:

[[clang::nomerge]] if (cond) {
   foo();
   [[clang::nomerge}] foo();
   foo(); // will not carry nomerge unless we restore to old value
}
clang/lib/CodeGen/CodeGenFunction.h
596

I would put your field after this one, and name it more specific, something like InNoMergeAttributedStmt. This field tracks a similar concept of being within a region

clang/test/CodeGen/attr-nomerge.cpp
7

I think the emergent behavior from the current implementation is reasonable. For lambdas, it applies to the outer call, not the inner lambda body.

I don't see any reason to make this work for global variables currently.

We could try to ban the application of this attribute to compound statements and statments containing them ({}, if, for), but that's a lot of effort. I could go either way.

zequanwu updated this revision to Diff 261973.May 4 2020, 5:38 PM
zequanwu marked 10 inline comments as done.

To keep it simple, nomerge attribute should not be applied to decl statement. Since label statement calls ProcessDeclAttributeList to handle attributes, label statement should not have nomerge attribute.

zequanwu added inline comments.May 4 2020, 5:39 PM
clang/lib/CodeGen/CGStmt.cpp
612

Not exist for statements.

rsmith added a subscriber: rsmith.May 4 2020, 6:02 PM
rsmith added inline comments.
clang/lib/Sema/SemaStmtAttr.cpp
178

This will recurse into too much (eg, the bodies of lambdas and blocks, and unevaluated operands). It would be better to make this a ConstEvaluatedExprVisitor instead (see include/clang/AST/EvaluatedExprVisitor.h).

zequanwu marked an inline comment as done.May 5 2020, 1:06 PM
zequanwu added inline comments.
clang/lib/Sema/SemaStmtAttr.cpp
178

Do you mean to create a subclass of ConstEvaluatedExprVisitor to find if the statement has call expression?

zequanwu updated this revision to Diff 262922.May 8 2020, 11:38 AM

use ConstStmtVisitor to do recursive lookup for call expression.

zequanwu marked an inline comment as done.May 8 2020, 11:39 AM
rsmith added inline comments.May 8 2020, 1:36 PM
clang/lib/Sema/SemaStmtAttr.cpp
178

This is still recursing into unevaluated operands (eg. the operand of sizeof). Please use ConstEvaluatedExprVisitor instead of ConstStmtVisitor.

zequanwu updated this revision to Diff 262971.May 8 2020, 4:04 PM
zequanwu marked an inline comment as done.

use ConstEvaluatedExprVisitor to recursively looking up for call expr

rnk accepted this revision.May 18 2020, 2:45 PM

I think this looks good and @zequanwu has addressed the concerns of other reviewers. Please wait until Wednesday before pushing in case they raise other concerns.

clang/lib/Sema/SemaStmtAttr.cpp
175

I guess I'm a little concerned that we have to break out CRTP and walk the whole sub statement to make this warning work, but we do that kind of stuff everywhere I guess. :( I wish we had a cheaper way to do this.

This revision is now accepted and ready to land.May 18 2020, 2:45 PM
zequanwu updated this revision to Diff 265648.May 21 2020, 5:04 PM
zequanwu retitled this revision from Add nomerge function attribute to clang to Add nomerge statement attribute to clang.
zequanwu edited the summary of this revision. (Show Details)

update test case.