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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
