Add nomerge statement attribute to clang

[zequanwu]

Summary

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

[rnk]

clang/include/clang/Basic/Attr.td

1799

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

16

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]

clang/include/clang/Basic/Attr.td

1799

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]

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

1799

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]

clang/include/clang/Basic/Attr.td

1799

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]

clang/include/clang/Basic/Attr.td

1799

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]

change nomerge to statement attribute

[aaron.ballman]

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

4528

Missing a full stop at the end of the comment.

clang/lib/CodeGen/CGStmt.cpp

611 ↗

(On Diff #261406)

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

clang/lib/Sema/SemaStmtAttr.cpp

175 ↗

(On Diff #261406)

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]

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

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2755–2756 ↗

(On Diff #261698)

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

clang/lib/Sema/SemaStmtAttr.cpp

173 ↗

(On Diff #261698)

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.

175–179 ↗

(On Diff #261698)

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

184 ↗

(On Diff #261698)

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

189 ↗

(On Diff #261698)

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

clang/test/CodeGen/attr-nomerge.cpp

6 ↗

(On Diff #261698)

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]

clang/lib/CodeGen/CGStmt.cpp

611 ↗

(On Diff #261698)

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

615 ↗

(On Diff #261698)

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

599 ↗

(On Diff #261698)

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

6 ↗

(On Diff #261698)

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]

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]

clang/lib/CodeGen/CGStmt.cpp

611 ↗

(On Diff #261698)

Not exist for statements.

[rsmith]

clang/lib/Sema/SemaStmtAttr.cpp

178 ↗

(On Diff #261973)

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]

clang/lib/Sema/SemaStmtAttr.cpp

178 ↗

(On Diff #261973)

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

[zequanwu]

use ConstStmtVisitor to do recursive lookup for call expression.

[rsmith]

clang/lib/Sema/SemaStmtAttr.cpp

178 ↗

(On Diff #261973)

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

[zequanwu]

use ConstEvaluatedExprVisitor to recursively looking up for call expr

[zequanwu]

Ping

[rnk]

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 ↗

(On Diff #262971)

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.

[zequanwu]

update test case.

[zequanwu]

landed

clang/include/clang/Basic/Attr.td

 }
 
 def NoReturn : InheritableAttr {
   let Spellings = [GCC<"noreturn">, Declspec<"noreturn">];
   // FIXME: Does GCC allow this on the function instead?
   let Documentation = [Undocumented];
 }
 
+def NoMerge : InheritableAttr {
+  let Spellings = [Clang<"nomerge">];
+  let Subjects = SubjectList<[FunctionLike]>;
+  let Documentation = [NoMergeDocs];
+  let SimpleHandler = 1;
+}
+
 def NoInstrumentFunction : InheritableAttr {
   let Spellings = [GCC<"no_instrument_function">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
 def NotTailCalled : InheritableAttr {

clang/include/clang/Basic/AttrDocs.td

def CXX11NoReturnDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 A function declared as ``[[noreturn]]`` shall not return to its caller. The
 compiler will generate a diagnostic for a function declared as ``[[noreturn]]``
 that appears to be capable of returning to its caller.
   }];
 }
 
+def NoMergeDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Calls to functions marked `nomerge` will not be merged during optimization.
+This attribute can be used to prevent the optimizer from obscuring the source
+location of certain calls. For example, it will prevent tail merging otherwise
+identical code sequences that raise an exception or terminate the program. Tail
+merging normally reduces the precision of source location information, making
+stack traces less useful for debugging. This attribute gives the user control
+over the tradeoff between code size and debug information precision.
+  }];
+}
+
 def AssertCapabilityDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "assert_capability, assert_shared_capability";
   let Content = [{
 Marks a function that dynamically tests whether a capability is held, and halts
 the program if it is not held.
   }];
 }

clang/lib/CodeGen/CGCall.cpp

if (TargetDecl) {
     if (TargetDecl->hasAttr<NoReturnAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
     if (TargetDecl->hasAttr<ColdAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::Cold);
     if (TargetDecl->hasAttr<NoDuplicateAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
     if (TargetDecl->hasAttr<ConvergentAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::Convergent);
+    if (TargetDecl->hasAttr<NoMergeAttr>())
+      FuncAttrs.addAttribute(llvm::Attribute::NoMerge);
 
     if (const FunctionDecl *Fn = dyn_cast<FunctionDecl>(TargetDecl)) {
       AddAttributesFromFunctionProtoType(
           getContext(), FuncAttrs, Fn->getType()->getAs<FunctionProtoType>());
       if (AttrOnCallSite && Fn->isReplaceableGlobalAllocationFunction()) {
         // A sane operator new returns a non-aliasing pointer.
         auto Kind = Fn->getDeclName().getCXXOverloadedOperator();
         if (getCodeGenOpts().AssumeSaneOperatorNew &&
#endif
 
   if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl))
     if (FD->usesFPIntrin())
       // All calls within a strictfp function are marked strictfp
       Attrs =
         Attrs.addAttribute(getLLVMContext(), llvm::AttributeList::FunctionIndex,
                            llvm::Attribute::StrictFP);
 
   // Apply some call-site-specific attributes.
   // TODO: work this into building the attribute set.
 
   // Apply always_inline to all calls within flatten functions.
   // FIXME: should this really take priority over __try, below?
   if (CurCodeDecl && CurCodeDecl->hasAttr<FlattenAttr>() &&
       !(TargetDecl && TargetDecl->hasAttr<NoInlineAttr>())) {
     Attrs =
         Attrs.addAttribute(getLLVMContext(), llvm::AttributeList::FunctionIndex,

clang/test/CodeGen/attr-nomerge.c

+// RUN: %clang_cc1 -S -emit-llvm %s -o - | FileCheck %s
+
+void bar() __attribute__((nomerge));
+
+void foo(int i) {
+  if (i == 5) {
+    bar();
+  } else if (i == 7) {
+    bar();
+  }
+  bar();
+}
+// CHECK: declare void @bar(...) #[[NOMERGEATTR:[0-9]+]]
+// CHECK: attributes #[[NOMERGEATTR]] = { nomerge {{.*}} }

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDuplicate (SubjectMatchRule_function)
 // CHECK-NEXT: NoEscape (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NoInline (SubjectMatchRule_function)
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function)
+// CHECK-NEXT: NoMerge (SubjectMatchRule_function)
 // CHECK-NEXT: NoMicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
 // CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSanitizeSpecific (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: NoSplitStack (SubjectMatchRule_function)
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)