[Clang] Make nomerge attribute a function attribute as well as a statement attribute.

[zequanwu]

Summary

Make nomerge attribute a function attribute as well as a statement attribute to extend the functionality of nomerge attribute.

For functions already have nomerge attribute, don't add nomerge attribute at the call-site of this function.

[zequanwu]

start a new diff to make nomerge attribute a function attribute as well as a statement attribute.

[rnk]

Nice, that wasn't too difficult. I had some suggestions for improving the test case, and I'd like to hear from Aaron.

clang/test/CodeGen/attr-nomerge.cpp

8

Hm, virtual functions, there's something worth thinking about. In this case, the attribute must appear on the call site if we want it to work as the user expects. I don't see the indirect call site for the virtual call in the IR below. I think it's worth testing for that. Consider taking A* and B* parameters to defeat any frontend devirtualization optimizations.

17

I would leave this declaration as it was, so that all the statement attribute test cases you wrote below remain as they were, and the attribute continues to appear at the call site.

The f declaration below gives you coverage of the free function declaration case.

[zequanwu]

Add test for virutal function.

clang/test/CodeGen/attr-nomerge.cpp

8

If devirtualization failed, then only statement attribute would work.

[zequanwu]

clang/test/CodeGen/attr-nomerge.cpp

17

I added [[clang::nomerge]] to bar in order to test that the generated IR should attach nomerge to call-sites which callee function already has this attribute.

[rnk]

clang/test/CodeGen/attr-nomerge.cpp

8

Fair enough, that is similar to how not_tail_called works. Can you also update AttrDocs.td to mention the limitation?

17

Well, from the tests below, it seems like the attribute is *not* present on the call sites. Is that what you intended?

[zequanwu]

Add docs.

clang/test/CodeGen/attr-nomerge.cpp

17

oops, I meant "shouldn't attach", because it's not necessary to have it.

[aaron.ballman]

In general, I think this is reasonable -- there's a bit more work to be done before it's ready to land, but this is heading in the right direction.

clang/include/clang/Basic/Attr.td

562

I think this should be an InheritableAttr, like DeclOrTypeAttr. With type/decl attributes, those are almost always for things like calling conventions or other cases where inheritance of the attribute is really likely to be the correct default. It's less clear to me that the same is true for stmt/decl attributes aside from the observation that most decl attributes are inheritable. We may need to come up with a better approach for inheritance here at some point when we find a stmt/decl attribute that should not be inheritable.

clang/lib/Sema/SemaDeclAttr.cpp

2260

No need for this function, you can set let SimpleHandler = 1; in Attr.td -- the common attribute handler code should handle checking the subject list for you automatically.

7914

Then this can become handleSimpleAttribute<NoMergeAttr>(S, D, AL);

clang/test/CodeGen/attr-nomerge.cpp

91

Can you also add a test case to demonstrate that inheriting the attribute on a later redeclaration works? It can either be a codegen test or an AST dump test.

clang/test/Sema/attr-nomerge.cpp

11

This diagnostic is no longer correct -- nomerge now applies to more things. You may need to teach the clang attribute emitter about stmt/decl attribute types so that it can generate a better diagnostic in diagnoseAppertainsTo().

[zequanwu]

address comments.

clang/test/CodeGen/attr-nomerge.cpp

91

Do you mean something like this?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

1326

Related to my comments in ClangAttrEmitter.cpp, I think you should make these changes instead.

clang/test/CodeGen/attr-nomerge.cpp

91

Close. I was thinking something like:

int g(int i); // No attr

void something() {
  // call g() here
}

[[clang::nomerge]] int g(int i);

void something_else() {
  // call g() here
}

int g(int i) { return i; }

void something_else_again() {
  // call g() here
}

So this tests that the attribute is inherited on redeclarations properly and it also tests the merging behavior when the declaration may not have yet had the attribute attached.

clang/utils/TableGen/ClangAttrEmitter.cpp

3449–3450

I think this will do the wrong thing when the subject list has more than one entry (I think this will add statements to the list once for each subject). As an experiment, can you add ObjCMethod to the list of subjects for NoMerge in Attr.td? Do the resulting diagnostics list "statements" once or twice?

Thinking a bit deeper on this -- I think my original advice may have been too ambitious (we don't yet have support for automatically grabbing the names of statements for diagnostics like we do for declarations). Trying to add that as part of this patch would be a big ask, so I think a better approach is to use a custom diagnostic in Attr.td. I'll add a comment there to this effect.

[zequanwu]

address comments.

clang/utils/TableGen/ClangAttrEmitter.cpp

3449–3450

You're right. If I just add ObjCMethod to the list of subjects for NoMerge, the "statements" shows twice in the diag.

[aaron.ballman]

LGTM! Thank you for this!