Page MenuHomePhabricator

Implement Attribute Target MultiVersioning (Improved edition!)
ClosedPublic

Authored by erichkeane on Dec 4 2017, 4:46 PM.

Details

Summary

GCC's attribute 'target', in addition to being an optimization hint,
also allows function multiversioning. We currently have the former
implemented, this is the latter's implementation.

This works by enabling functions with the same name/signature to coexist,
so that they can all be emitted. Multiversion state is stored in the
FunctionDecl itself, and SemaDecl manages the definitions.
Note that it ends up having to permit redefinition of functions so
that they can all be emitted. Additionally, all versions of the function
must be emitted, so this also manages that.

Note that this includes some additional rules that GCC does not, since
defining something as a MultiVersion function after a usage has been made illegal.

The only 'history rewriting' that happens is if a function is emitted before
it has been converted to a multiversion'ed function, at which point its name
needs to be changed.

Function templates and virtual functions are NOT yet supported (not supported
in GCC either).

This SEMA design was discussed with @rsmith but additional opinions/preferences
here are greatly appreciated.

Options on how to split this patch up would also be particularly solicited,
since this IS a large patch.

This patch completely superceeds: https://reviews.llvm.org/D38596

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Dec 13 2017, 7:54 AM
lib/CodeGen/CodeGenFunction.cpp
2311–2312

You can use llvm::for_each instead.

lib/CodeGen/CodeGenModule.cpp
750

Might as well sink this into the call to std::sort().

lib/Sema/SemaDecl.cpp
9168

Don't use auto here as the type is not explicitly spelled out in the initialization.

9196

This function uses quite a few magic numbers that might be better expressed with named values instead.

9232–9234

Formatting is off here.

9233

Can use const auto * here.

9251

Can use const auto * here.

9318

So any other form of target attribute and feature string is fine to place on main()?

9348

Do not use auto here.

9364

Capitalization.

9432

Do not use auto here.

9447

Given how often the features need to be sorted, would it make sense to hoist this functionality into parse()?

erichkeane marked 17 inline comments as done.Dec 13 2017, 10:19 AM

Patch incoming, Thank you very much for the review @aaron.ballman

include/clang/Basic/DiagnosticSemaKinds.td
9350–9352

I tried again, hopefully this one is better? :) Let me know if my grammar is still too good...

9356–9357

This attribute can have negative effects if combined with certain others, though I don't have a complete list. GCC seems to 'ignore' a handful of others, but I don't have the complete list. It is my intent to disallow all others in this patch, and upon request/further work, permit them as an opt-in.

lib/Sema/SemaDecl.cpp
9318

It IS, it just doesn't cause multiversioning. The "return false" at the bottom of this condition returns to CheckFunctionDeclaration.

erichkeane marked 2 inline comments as done.

Fixed all of @aaron.ballman comments.

@aaron.ballman reminded me that this should only work on ELF targets, so this patch enforces that constraint.

aaron.ballman added inline comments.Dec 14 2017, 11:25 AM
include/clang/Basic/DiagnosticSemaKinds.td
9350

instead of duplicate, do you mean redeclarations? Or do you mean overloaded declarations? Both?

9350–9352

You've achieved sufficiently incorrect grammar. ;-)

erichkeane added inline comments.Dec 14 2017, 11:36 AM
include/clang/Basic/DiagnosticSemaKinds.td
9350

I do mean redeclarations, thanks! Change incoming.

Fix error message per @aaron.ballman s suggestion.

I'm sure most of you have taken off for the year, but if anyone had time, *ping* :D

Hi all-- I'm intending to miss the branch for 6.0, but I'd love to get this in soon after. Can anyone take another look?

Thanks,
Erich

echristo accepted this revision.Jan 3 2018, 6:32 PM

Couple of inline comments, otherwise I'm pretty happy. I'd wait for an ack by Richard for this though.

-eric

lib/CodeGen/CGBuiltin.cpp
7673 ↗(On Diff #127005)

Why do you need to pass in a Builder?

lib/CodeGen/CodeGenFunction.cpp
2328

Can you get here via trying to compile code for another cpu?

This revision is now accepted and ready to land.Jan 3 2018, 6:32 PM
rsmith added a comment.Jan 3 2018, 7:00 PM

Lots of comments, but no high-level design concerns. I think this is very close to being ready to go.

include/clang/AST/Decl.h
2164–2170

Instead of looping over redeclarations here, how about only storing the flag on the canonical declaration (and only looking there in isMultiVersion)?

include/clang/Basic/Attr.td
1847

Nit: space before {.

include/clang/Basic/DiagnosticSemaKinds.td
9336–9337

Is it "multiversion function" or "multiversioned function"? You're using both in these diagnostics; please pick one and use it consistently. I prefer the "-ed" form, but I'm happy with whichever you'd prefer.

9339

"multiversioning" maybe? (Again, this is used inconsistently; there's a "function multiversioning" in err_multiversion_not_supported).

9346–9348

Maybe we should just suppress the "candidate" note entirely for these cases?

lib/CodeGen/CodeGenModule.cpp
898–899

I'm not especially enamoured with getMangledName mutating the IR. Can we perform this rename as part of emitting the multiversion dispatcher or ifunc, rather than here? (Or do we really not have anywhere else that this can live?)

2061–2062

Please add a comment explaining this check. (I'm guessing the issue is that we can't form a cross-translation-unit reference to the right function from an IFUNC, so we can't rely on an available_externally definition actually being usable? But it's not clear to me that this is the right resolution to that, especially in light of the dllexport case below where it would not be correct to emit the definition.)

2143–2146

Consider moving the lookup, loop and type check here into an ASTContext or FunctionDecl utility to find or visit all the versions of a multiversioned function.

2145

if statements with nontrivial bodies should have their own braces.

2146

Yes, the lookup can in some cases find multiple declarations from the same redeclaration chain. (This happens particularly when the declarations are loaded from AST files.)

lib/Sema/SemaDecl.cpp
9396

Do not hardcode English text as a diagnostic parameter. And don't reuse a diagnostic to mean something else :) The %0 in this diagnostic is for the name of the declaration.

diag::note_previous_declaration seems to capture what you're actually looking for here. (It's a note for "previous declaration of the same entity", whereas note_previous_decl is a note for "here's some previous declaration of something else that's relevant").

9401–9402

This doesn't seem right: the OldFD isn't (necessarily) a declaration without a prototype.

9423

As above.

9477

As above.

9601

The parameter in CheckMultiVersionFunction corresponding to MergeTypeWithPrevious here is called MayNeedOverloadableChecks, which is also the name of a local variable in this function. Did you pass the wrong bool? Or is the name of the parameter to CheckMultiVersionFunction wrong?

lib/Sema/SemaOverload.cpp
5963

Please move this comparison of getFeaturesStr() against "default" into a dedicated function on TargetAttr (::isDefaultVersion()?). This magic string literal is appearing quite a lot in this patch...

lib/Serialization/ASTReaderDecl.cpp
2821

This comment is missing a

2825

This assertion will fire if X is multiversion but Y is not. It's probably not too crucial what happens in that case, but we shouldn't assert. An error would be nice, but we can't produce one from here, so how about we just say that if one is multiversion and the other is not, that they're considered to not be the same entity.

2827

Space after if, please.

test/CodeGen/attr-target-mv-func-ptrs.c
11–16

What about uses in contexts where there is no target function type? For example, +foo;

test/CodeGenCXX/attr-target-mv-member-funcs.cpp
4–9

OK, multiversioned member functions! Let's look at some nasty corner cases!

Do you allow multiversioning of special member functions (copy constructor, destructor, ...)? Some tests for that would be interesting. Note in particular that CXXRecordDecl::getDestructor assumes that there is only one destructor for a class, and I expect we make that assumption in a bunch of other places too. Might be best to disallow multiversioning destructors for now.

Do you allow a multiversioned function to have one defaulted version and one non-defaulted version? What does that mean for the properties of the class that depend on whether special member functions are trivial? Might be a good idea to disallow defaulting a multiversioned function. Oh, and we should probably not permit versions of a multiversioned function to be deleted either.

If you allow multiversioning of constructors, I'd like to see a test for multiversioning of inherited constructors. Likewise, if you allow multiversioning of conversion functions, I'd like to see a test for that. (I actually think there's a very good chance both of those will just work fine.)

You don't allow multiversioning of function templates right now, but what about multiversioning of member functions of a class template? Does that work? If so, does class template argument deduction using multiversioned constructors work?

Does befriending multiversioned functions work? Is the target attribute taken into account?

erichkeane marked 34 inline comments as done.Jan 5 2018, 12:00 PM

Patch incoming, sorry it took so long!

lib/CodeGen/CGBuiltin.cpp
7673 ↗(On Diff #127005)

Because when I first wrote this, the EmitResolver function was a free function. I guess I missed this dependency along the way, thanks for the catch!

lib/CodeGen/CodeGenFunction.cpp
2328

At the moment, no. I'm asserting to make sure that is the case, and to be a hint for George/et-al who are implementing this in the future for other processors.

lib/CodeGen/CodeGenModule.cpp
898–899

Unfortunately it is too late at that point. The problem is you have:

TARGET_SSE MVFunc();
TARGET_DEF MVFunc(); // At this point, a mangling-conflict occurs.

void foo() {
MVFunc(); // Only at THIS point does the IFunc get created, too late to rewrite the SSE variant's name.
}

That said, I moved it to a more appropriate place.

2061–2062

The actual case I encountered was that when emitting the global in emitMultiVersionFunctions, the EmitGlobalDefinition was immediately skipping it here because it was an inline function.

I believe the correct response is to just call EmitGlobalFunctionDefinition from the emitMultiVersionFunctions handler. This will have to be modified when I support virtual functions or constructors, but this will make it work.

IMO, the cross-TU issue you come up with would only be an issue when the value isn't emitted due to it being inline/static/etc. In this case, the failed linking is an acceptable consequence to the user improperly providing a multiversion variant.

lib/Sema/SemaDecl.cpp
9601

Looks like I'd just grabbed the wrong name for the parameter in CheckMultiVersionFunction. Fixed!

test/CodeGen/attr-target-mv-func-ptrs.c
11–16

Added as a test in Sema/attr-target-mv.c.

test/CodeGenCXX/attr-target-mv-member-funcs.cpp
4–9

I gave CTORs a try, and found that there are a few subtle issues that need to be dealt with in code-gen. For the moment (and since GCC simply terminates if you try to use them), I'd like to disallow them.

Definitely going to disallow dtors/default/deleted functions.

erichkeane marked 7 inline comments as done.

Fixes for all @echristo and @rsmith s comments.

erichkeane planned changes to this revision.Jan 5 2018, 1:54 PM

Forgot friend functions, Working on it now, sorry about that.

erichkeane updated this revision to Diff 128792.Jan 5 2018, 1:57 PM

Added test for friend functions.

This revision is now accepted and ready to land.Jan 5 2018, 1:57 PM
erichkeane updated this revision to Diff 128793.Jan 5 2018, 1:59 PM

Sorry for the thrash, thought of a better way to do the test just as I clicked 'save' last time :)

rsmith added a comment.Jan 5 2018, 6:40 PM

One other test I'd like to see is what happens if you declare a version before the first use, and define it afterwards, particularly if the version is an inline function:

inline __attribute__((target("default"))) void f();
inline __attribute__((target("foo"))) void f();
void g() { f(); }
__attribute__((target("default"))) void f() {}
__attribute__((target("foo"))) void f() {}
include/clang/AST/ASTContext.h
2643–2648

This should deal with the case where lookup finds multiple declarations of the same version (which can happen in particular when serializing to AST files; we keep around a handle to the version from each AST file). Also, consider moving this to the .cpp file (use llvm::function_ref<void(const FunctionDecl*)> to pass in the callback).

include/clang/Basic/Attr.td
1847

Space before { seems to have been removed again. Clang-format really doesn't like tablegen files :)

include/clang/Basic/DiagnosticSemaKinds.td
9354–9356

"multiversioned function declaration has a different destructors" doesn't sound right to me :)

lib/Sema/SemaDecl.cpp
9228–9229

It seems strange to have a "caused here" note pointing at the same location that you just produced an error on. Is this note really adding value? (Likewise on all later diagnostics that add this note at the same place as the error.)

lib/Sema/SemaOverload.cpp
10220

Couple of typos here.

lib/Serialization/ASTReaderDecl.cpp
2825

This is still not quite right for the case of a multiversion / non-multiversion mismatch: you don't return 'false' when Y is multiversion and X is not.

test/CodeGenCXX/attr-target-mv-constexpr.cpp
1–16 ↗(On Diff #128793)

Can you move this to test/SemaCXX and change it to only test the constant evaluation side of things (eg, static_assert(foo() == 2))?

erichkeane marked 7 inline comments as done.Jan 8 2018, 10:15 AM

patch incoming!

include/clang/AST/ASTContext.h
2643–2648

Ah, right, this part got dropped when I was moving it around...

There is only 2 ways I could think of to do this properly, first is to actually check the 'target' strings, second is to check the address of the canonical decl. Checking target strings seems expensive, but I don't know if the pointer values are sufficient?

I'll implement the 2nd for this patch, but if you could give me a hint as the proper way (if this isn't sufficient), I'd be grateful.

lib/Sema/SemaDecl.cpp
9228–9229

I think that makes sense. I'll remove all of those diagnostics where the error and the 'caused by' are different lines.

erichkeane marked 2 inline comments as done.

All of @rsmith 's comments on the last patch. 1 "open" item on the duplicate discover in the resolver generation, but hopefully this otherwise acceptable.

rsmith accepted this revision.Jan 8 2018, 11:32 AM

Looks good to me with just a few more tweaks (assuming these comments don't uncover any new issues). Thank you!

lib/AST/ASTContext.cpp
9490

Consider using SmallDenseSet here. (In the modules case, we could potentially have hundreds or even thousands of lookup results, and the better asymptotics in the bad cases seems on balance worth the higher constant factor.)

Also, you'll need to add declarations to this set :)

A testcase for this would look something like:

#pragma clang module build A
module A {}
#pragma clang module contents
#pragma clang module begin A
__attribute__((target("default"))) void f();
__attribute__((target("foo"))) void f();
#pragma clang module end
#pragma clang module endbuild

#pragma clang module build B
module B {}
#pragma clang module contents
#pragma clang module begin B
__attribute__((target("default"))) void f();
__attribute__((target("foo"))) void f();
#pragma clang module end
#pragma clang module endbuild

#pragma clang module import A
#pragma clang module import B
void g() { f(); }

... with a check that the resolver ifunc only includes each case once.

lib/Serialization/ASTReaderDecl.cpp
2832

The !TAY check here is now redundant and can be moved into the assertion.

test/CodeGen/attr-target-mv.c
20–26

This will either need to be a C++ test or will need inline on the definitions to have the intended effect of creating a discardable function definition.

test/CodeGenCXX/attr-target-mv-member-funcs.cpp
4–9

Can you also test that out-of-line definitions of multiversioned member functions are properly matched with the in-class declaration, and that you can't add new versions not present in the class definition that way? (Again, I'm not expecting any problems here, but it seems like something we should have coverage for.)

erichkeane closed this revision.Jan 8 2018, 1:35 PM
erichkeane marked 2 inline comments as done.

Closed with revision 322028