[PATCH] D79121: Add nomerge function attribute to clang

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:611
 void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
+  for (const auto *A: S.getAttrs())
+    if (A->getKind() == attr::NoMerge) {
----------------
Can we use S.hasAttr, or does that not exist for statements?


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:615
+      break;
+    }
   EmitStmt(S.getSubStmt(), S.getAttrs());
----------------
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
  }


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:599
   /// region.
   bool IsInPreservedAIRegion = false;
 
----------------
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


================
Comment at: clang/test/CodeGen/attr-nomerge.cpp:6
+
+void foo(int i) {
+  [[clang::nomerge]] bar();
----------------
aaron.ballman wrote:
> 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()) {}
> }
> ```
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79121/new/

https://reviews.llvm.org/D79121