Page MenuHomePhabricator

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

Authored by zequanwu on Dec 7 2020, 4:53 PM.

Details

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.

Diff Detail

Event Timeline

zequanwu created this revision.Dec 7 2020, 4:53 PM
zequanwu requested review of this revision.Dec 7 2020, 4:53 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 7 2020, 4:53 PM
zequanwu updated this revision to Diff 310760.Dec 9 2020, 9:14 PM

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

zequanwu retitled this revision from [Clang] Add disable-merge function attribute which generates nomerge function attribute in IR to [Clang] Make nomerge attribute a function attribute as well as a statement attribute..Dec 9 2020, 9:16 PM
zequanwu edited the summary of this revision. (Show Details)
zequanwu edited the summary of this revision. (Show Details)
rnk added a comment.Dec 10 2020, 11:40 AM

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 updated this revision to Diff 311042.Dec 10 2020, 3:39 PM
zequanwu marked an inline comment as done.

Add test for virutal function.

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

If devirtualization failed, then only statement attribute would work.

zequanwu added inline comments.Dec 10 2020, 3:41 PM
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 added inline comments.Dec 10 2020, 3:48 PM
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 updated this revision to Diff 311062.Dec 10 2020, 4:37 PM
zequanwu marked an inline comment as done.

Add docs.

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

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

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 updated this revision to Diff 312078.Dec 15 2020, 5:01 PM
zequanwu marked 4 inline comments as done.

address comments.

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

Do you mean something like this?

aaron.ballman added inline comments.Dec 16 2020, 6:13 AM
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 updated this revision to Diff 312348.Dec 16 2020, 5:35 PM
zequanwu marked 2 inline comments as done.

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 accepted this revision.Dec 17 2020, 5:23 AM

LGTM! Thank you for this!

This revision is now accepted and ready to land.Dec 17 2020, 5:23 AM
This revision was landed with ongoing or failed builds.Dec 17 2020, 7:46 AM
This revision was automatically updated to reflect the committed changes.