[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
This revision was automatically updated to reflect the committed changes.
Closed by commit rL371586: [CodeGen] Add alias for cpu_dispatch function with
IFunc Fix resolver linkage… (authored by MaskRay, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D67058?vs=219568=219651#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
Files:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/test/CodeGen/attr-cpuspecific.c
cfe/trunk/test/CodeGen/attr-target-mv-func-ptrs.c
cfe/trunk/test/CodeGen/attr-target-mv-va-args.c
cfe/trunk/test/CodeGen/attr-target-mv.c
cfe/trunk/test/CodeGenCXX/attr-cpuspecific.cpp
cfe/trunk/test/CodeGenCXX/attr-target-mv-diff-ns.cpp
cfe/trunk/test/CodeGenCXX/attr-target-mv-inalloca.cpp
cfe/trunk/test/CodeGenCXX/attr-target-mv-member-funcs.cpp
cfe/trunk/test/CodeGenCXX/attr-target-mv-modules.cpp
cfe/trunk/test/CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
cfe/trunk/test/CodeGenCXX/attr-target-mv-overloads.cpp
Index: cfe/trunk/test/CodeGen/attr-target-mv.c
===
--- cfe/trunk/test/CodeGen/attr-target-mv.c
+++ cfe/trunk/test/CodeGen/attr-target-mv.c
@@ -47,12 +47,12 @@
fwd_decl_avx();
}
-// LINUX: @foo.ifunc = ifunc i32 (), i32 ()* ()* @foo.resolver
-// LINUX: @foo_inline.ifunc = ifunc i32 (), i32 ()* ()* @foo_inline.resolver
-// LINUX: @foo_decls.ifunc = ifunc void (), void ()* ()* @foo_decls.resolver
-// LINUX: @foo_multi.ifunc = ifunc void (i32, double), void (i32, double)* ()* @foo_multi.resolver
-// LINUX: @fwd_decl_default.ifunc = ifunc i32 (), i32 ()* ()* @fwd_decl_default.resolver
-// LINUX: @fwd_decl_avx.ifunc = ifunc i32 (), i32 ()* ()* @fwd_decl_avx.resolver
+// LINUX: @foo.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @foo.resolver
+// LINUX: @foo_inline.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @foo_inline.resolver
+// LINUX: @foo_decls.ifunc = weak_odr ifunc void (), void ()* ()* @foo_decls.resolver
+// LINUX: @foo_multi.ifunc = weak_odr ifunc void (i32, double), void (i32, double)* ()* @foo_multi.resolver
+// LINUX: @fwd_decl_default.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @fwd_decl_default.resolver
+// LINUX: @fwd_decl_avx.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @fwd_decl_avx.resolver
// LINUX: define i32 @foo.sse4.2()
// LINUX: ret i32 0
@@ -72,14 +72,14 @@
// WINDOWS: define dso_local i32 @bar()
// WINDOWS: call i32 @foo.resolver()
-// LINUX: define i32 ()* @foo.resolver() comdat
+// LINUX: define weak_odr i32 ()* @foo.resolver() comdat
// LINUX: call void @__cpu_indicator_init()
// LINUX: ret i32 ()* @foo.arch_sandybridge
// LINUX: ret i32 ()* @foo.arch_ivybridge
// LINUX: ret i32 ()* @foo.sse4.2
// LINUX: ret i32 ()* @foo
-// WINDOWS: define dso_local i32 @foo.resolver() comdat
+// WINDOWS: define weak_odr dso_local i32 @foo.resolver() comdat
// WINDOWS: call void @__cpu_indicator_init()
// WINDOWS: call i32 @foo.arch_sandybridge
// WINDOWS: call i32 @foo.arch_ivybridge
@@ -92,14 +92,14 @@
// WINDOWS: define dso_local i32 @bar2()
// WINDOWS: call i32 @foo_inline.resolver()
-// LINUX: define i32 ()* @foo_inline.resolver() comdat
+// LINUX: define weak_odr i32 ()* @foo_inline.resolver() comdat
// LINUX: call void @__cpu_indicator_init()
// LINUX: ret i32 ()* @foo_inline.arch_sandybridge
// LINUX: ret i32 ()* @foo_inline.arch_ivybridge
// LINUX: ret i32 ()* @foo_inline.sse4.2
// LINUX: ret i32 ()* @foo_inline
-// WINDOWS: define dso_local i32 @foo_inline.resolver() comdat
+// WINDOWS: define weak_odr dso_local i32 @foo_inline.resolver() comdat
// WINDOWS: call void @__cpu_indicator_init()
// WINDOWS: call i32 @foo_inline.arch_sandybridge
// WINDOWS: call i32 @foo_inline.arch_ivybridge
@@ -112,11 +112,11 @@
// WINDOWS: define dso_local void @bar3()
// WINDOWS: call void @foo_decls.resolver()
-// LINUX: define void ()* @foo_decls.resolver() comdat
+// LINUX: define weak_odr void ()* @foo_decls.resolver() comdat
// LINUX: ret void ()* @foo_decls.sse4.2
// LINUX: ret void ()* @foo_decls
-// WINDOWS: define dso_local void @foo_decls.resolver() comdat
+// WINDOWS: define weak_odr dso_local void @foo_decls.resolver() comdat
// WINDOWS: call void @foo_decls.sse4.2
// WINDOWS: call void @foo_decls
@@ -126,7 +126,7 @@
// WINDOWS: define dso_local void @bar4()
// WINDOWS: call void @foo_multi.resolver(i32 1, double 5.{{[0+e]*}})
-// LINUX: define void (i32, double)* @foo_multi.resolver() comdat
+// LINUX: define weak_odr void (i32, double)* @foo_multi.resolver() comdat
// LINUX: and i32 %{{.*}}, 4352
// LINUX: icmp eq i32 %{{.*}}, 4352
// LINUX: ret void (i32, double)* @foo_multi.fma4_sse4.2
@@ -139,7 +139,7 @@
// LINUX: ret void (i32, double)* @foo_multi.avx_sse4.2
// LINUX: ret void (i32, double)* @foo_multi
-// WINDOWS: define dso_local void @foo_multi.resolver(i32 %0,
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn marked 18 inline comments as done. zsrkmyn added a comment. All done IMO. :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67058/new/ https://reviews.llvm.org/D67058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn updated this revision to Diff 219568.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
Files:
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGen/attr-cpuspecific.c
clang/test/CodeGen/attr-target-mv-func-ptrs.c
clang/test/CodeGen/attr-target-mv-va-args.c
clang/test/CodeGen/attr-target-mv.c
clang/test/CodeGenCXX/attr-cpuspecific.cpp
clang/test/CodeGenCXX/attr-target-mv-diff-ns.cpp
clang/test/CodeGenCXX/attr-target-mv-inalloca.cpp
clang/test/CodeGenCXX/attr-target-mv-member-funcs.cpp
clang/test/CodeGenCXX/attr-target-mv-modules.cpp
clang/test/CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
clang/test/CodeGenCXX/attr-target-mv-overloads.cpp
Index: clang/test/CodeGenCXX/attr-target-mv-overloads.cpp
===
--- clang/test/CodeGenCXX/attr-target-mv-overloads.cpp
+++ clang/test/CodeGenCXX/attr-target-mv-overloads.cpp
@@ -14,8 +14,8 @@
return foo_overload() + foo_overload(1);
}
-// LINUX: @_Z12foo_overloadv.ifunc = ifunc i32 (), i32 ()* ()* @_Z12foo_overloadv.resolver
-// LINUX: @_Z12foo_overloadi.ifunc = ifunc i32 (i32), i32 (i32)* ()* @_Z12foo_overloadi.resolver
+// LINUX: @_Z12foo_overloadv.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @_Z12foo_overloadv.resolver
+// LINUX: @_Z12foo_overloadi.ifunc = weak_odr ifunc i32 (i32), i32 (i32)* ()* @_Z12foo_overloadi.resolver
// LINUX: define i32 @_Z12foo_overloadi.sse4.2(i32 %0)
// LINUX: ret i32 0
@@ -51,25 +51,25 @@
// WINDOWS: call i32 @"?foo_overload@@YAHXZ.resolver"()
// WINDOWS: call i32 @"?foo_overload@@YAHH@Z.resolver"(i32 1)
-// LINUX: define i32 ()* @_Z12foo_overloadv.resolver() comdat
+// LINUX: define weak_odr i32 ()* @_Z12foo_overloadv.resolver() comdat
// LINUX: ret i32 ()* @_Z12foo_overloadv.arch_sandybridge
// LINUX: ret i32 ()* @_Z12foo_overloadv.arch_ivybridge
// LINUX: ret i32 ()* @_Z12foo_overloadv.sse4.2
// LINUX: ret i32 ()* @_Z12foo_overloadv
-// WINDOWS: define dso_local i32 @"?foo_overload@@YAHXZ.resolver"() comdat
+// WINDOWS: define weak_odr dso_local i32 @"?foo_overload@@YAHXZ.resolver"() comdat
// WINDOWS: call i32 @"?foo_overload@@YAHXZ.arch_sandybridge"
// WINDOWS: call i32 @"?foo_overload@@YAHXZ.arch_ivybridge"
// WINDOWS: call i32 @"?foo_overload@@YAHXZ.sse4.2"
// WINDOWS: call i32 @"?foo_overload@@YAHXZ"
-// LINUX: define i32 (i32)* @_Z12foo_overloadi.resolver() comdat
+// LINUX: define weak_odr i32 (i32)* @_Z12foo_overloadi.resolver() comdat
// LINUX: ret i32 (i32)* @_Z12foo_overloadi.arch_sandybridge
// LINUX: ret i32 (i32)* @_Z12foo_overloadi.arch_ivybridge
// LINUX: ret i32 (i32)* @_Z12foo_overloadi.sse4.2
// LINUX: ret i32 (i32)* @_Z12foo_overloadi
-// WINDOWS: define dso_local i32 @"?foo_overload@@YAHH@Z.resolver"(i32 %0) comdat
+// WINDOWS: define weak_odr dso_local i32 @"?foo_overload@@YAHH@Z.resolver"(i32 %0) comdat
// WINDOWS: call i32 @"?foo_overload@@YAHH@Z.arch_sandybridge"
// WINDOWS: call i32 @"?foo_overload@@YAHH@Z.arch_ivybridge"
// WINDOWS: call i32 @"?foo_overload@@YAHH@Z.sse4.2"
Index: clang/test/CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
===
--- clang/test/CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
+++ clang/test/CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
@@ -16,7 +16,7 @@
return s.foo(0);
}
-// LINUX: @_ZN1S3fooEi.ifunc = ifunc i32 (%struct.S*, i32), i32 (%struct.S*, i32)* ()* @_ZN1S3fooEi.resolver
+// LINUX: @_ZN1S3fooEi.ifunc = weak_odr ifunc i32 (%struct.S*, i32), i32 (%struct.S*, i32)* ()* @_ZN1S3fooEi.resolver
// LINUX: define i32 @_ZN1S3fooEi(%struct.S* %this, i32 %0)
// LINUX: ret i32 2
@@ -44,13 +44,13 @@
// WINDOWS: %s = alloca %struct.S, align 1
// WINDOWS: %call = call i32 @"?foo@S@@QEAAHH@Z.resolver"(%struct.S* %s, i32 0)
-// LINUX: define i32 (%struct.S*, i32)* @_ZN1S3fooEi.resolver() comdat
+// LINUX: define weak_odr i32 (%struct.S*, i32)* @_ZN1S3fooEi.resolver() comdat
// LINUX: ret i32 (%struct.S*, i32)* @_ZN1S3fooEi.arch_sandybridge
// LINUX: ret i32 (%struct.S*, i32)* @_ZN1S3fooEi.arch_ivybridge
// LINUX: ret i32 (%struct.S*, i32)* @_ZN1S3fooEi.sse4.2
// LINUX: ret i32 (%struct.S*, i32)* @_ZN1S3fooEi
-// WINDOWS: define dso_local i32 @"?foo@S@@QEAAHH@Z.resolver"(%struct.S* %0, i32 %1) comdat
+// WINDOWS: define weak_odr dso_local i32 @"?foo@S@@QEAAHH@Z.resolver"(%struct.S* %0, i32 %1) comdat
// WINDOWS: call i32 @"?foo@S@@QEAAHH@Z.arch_sandybridge"(%struct.S* %0, i32 %1)
// WINDOWS: call i32 @"?foo@S@@QEAAHH@Z.arch_ivybridge"(%struct.S* %0, i32 %1)
// WINDOWS: call i32 @"?foo@S@@QEAAHH@Z.sse4.2"(%struct.S* %0, i32 %1)
Index: clang/test/CodeGenCXX/attr-target-mv-modules.cpp
===
--- clang/test/CodeGenCXX/attr-target-mv-modules.cpp
+++ clang/test/CodeGenCXX/attr-target-mv-modules.cpp
@@ -22,7 +22,7 @@
void g() { f(); }
// Negative tests to validate that the
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
zsrkmyn wrote:
> erichkeane wrote:
> > zsrkmyn wrote:
> > > erichkeane wrote:
> > > > zsrkmyn wrote:
> > > > > erichkeane wrote:
> > > > > > zsrkmyn wrote:
> > > > > > > zsrkmyn wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > zsrkmyn wrote:
> > > > > > > > > > erichkeane wrote:
> > > > > > > > > > > This Resolver should have the same linkage as below.
> > > > > > > > > > Actually, I wanted to set linkage here at the first time,
> > > > > > > > > > but failed. When compiling code with cpu_specific but no
> > > > > > > > > > cpu_dispatch, we cannot set it as LinkOnceODR or WeakODR.
> > > > > > > > > > E.g.:
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > $ cat specific_only.c
> > > > > > > > > > __declspec(cpu_specific(pentium_iii))
> > > > > > > > > > int foo(void) { return 0; }
> > > > > > > > > > int usage() { return foo(); }
> > > > > > > > > >
> > > > > > > > > > $ clang -fdeclspec specific_only.c
> > > > > > > > > >
> > > > > > > > > > Global is external, but doesn't have external or weak
> > > > > > > > > > linkage!
> > > > > > > > > >
> > > > > > > > > > i32 ()* ()* @foo.resolver
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > fatal error: error in backend: Broken module found,
> > > > > > > > > > compilation aborted!
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c,
> > > > > > > > > > in which 'SingleVersion()' doesn't have a cpu_dispatch
> > > > > > > > > > declaration.
> > > > > > > > > The crash message is complaining it isn't external/weak.
> > > > > > > > > However, WeakODR should count, right? Can you look into it a
> > > > > > > > > bit more to see what it thinks is broken?
> > > > > > > > No, actually I've tried it earlier with the example I mentioned
> > > > > > > > in my last comment, but WeakODR still makes compiler
> > > > > > > > complaining. I think it's `foo.resolver` that cannot be
> > > > > > > > declared with as WeakODR/LinkOnceODR without definition. But
> > > > > > > > I'm really not familiar with these rules.
> > > > > > > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > > > > > > declaration can only be `ExternalLinkage` or
> > > > > > > `ExternalWeakLinkage`. So I still believe we cannot set the
> > > > > > > resolver to `LinkOnceODRLinkage/WeakODRLinkage` here, as they are
> > > > > > > declared but not defined when we only have `cpu_specified` but no
> > > > > > > `cpu_dispatch` in a TU as the example above.
> > > > > > That doesn't seem right then. IF it allows ExternalWeakLinkage I'd
> > > > > > expect WeakODR to work as well, since it is essentially the same
> > > > > > thing.
> > > > > I think we should have a double check. It is said "It is illegal for
> > > > > a function declaration to have any linkage type other than `external`
> > > > > or `extern_weak`" at the last line of section Linkage Type in the
> > > > > reference manual [1]. I guess `weak_odr` is not designed for
> > > > > declaration purpose and should be only used by definition.
> > > > >
> > > > > [1] https://llvm.org/docs/LangRef.html#linkage-types
> > > > I had typed a reply, but apparently it didn't submit: Ah, nvm, I see
> > > > now that external-weak is different from weak.
> > > >
> > > > I don't really get the linkages sufficiently to know what the right
> > > > thing to do is then. If we DO have a definition, I'd say weak_odr so
> > > > it can be merged, right? If we do NOT, could externally_available work?
> > > No, I think it should be `external` instead of `available_externally`.
> > > The later cannot used for declaration as well.
> > >
> > > So, getting back to the example, **1)** if we have `cpu_dispatch` and
> > > `cpu_specific` in same TU, it's okay to use `weak_odr` for `foo.resolver`
> > > as it is defined when `emitCPUDispatchDefinition` and it can be merged.
> > > **2)** If we only have `cpu_specific` in a TU and have a reference to the
> > > dispatched function, `foo.resolver` will be referenced without
> > > definition, and `external` is the proper linkage to make it work.
> > >
> > > That's why I didn't set linkage type at this line.
> > > No, I think it should be external instead of available_externally. The
> > > later cannot used for declaration as well.
> >
> > Wouldn't that make it un-mergable later? Meaning, if you emitted the
> > declaration from one TU, and the definition from another that you'd get a
> > link error?
> >
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
erichkeane wrote:
> zsrkmyn wrote:
> > erichkeane wrote:
> > > zsrkmyn wrote:
> > > > erichkeane wrote:
> > > > > zsrkmyn wrote:
> > > > > > zsrkmyn wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > zsrkmyn wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > This Resolver should have the same linkage as below.
> > > > > > > > > Actually, I wanted to set linkage here at the first time, but
> > > > > > > > > failed. When compiling code with cpu_specific but no
> > > > > > > > > cpu_dispatch, we cannot set it as LinkOnceODR or WeakODR.
> > > > > > > > > E.g.:
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > > $ cat specific_only.c
> > > > > > > > > __declspec(cpu_specific(pentium_iii))
> > > > > > > > > int foo(void) { return 0; }
> > > > > > > > > int usage() { return foo(); }
> > > > > > > > >
> > > > > > > > > $ clang -fdeclspec specific_only.c
> > > > > > > > >
> > > > > > > > > Global is external, but doesn't have external or weak
> > > > > > > > > linkage!
> > > > > > > > >
> > > > > > > > > i32 ()* ()* @foo.resolver
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > fatal error: error in backend: Broken module found,
> > > > > > > > > compilation aborted!
> > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in
> > > > > > > > > which 'SingleVersion()' doesn't have a cpu_dispatch
> > > > > > > > > declaration.
> > > > > > > > The crash message is complaining it isn't external/weak.
> > > > > > > > However, WeakODR should count, right? Can you look into it a
> > > > > > > > bit more to see what it thinks is broken?
> > > > > > > No, actually I've tried it earlier with the example I mentioned
> > > > > > > in my last comment, but WeakODR still makes compiler complaining.
> > > > > > > I think it's `foo.resolver` that cannot be declared with as
> > > > > > > WeakODR/LinkOnceODR without definition. But I'm really not
> > > > > > > familiar with these rules.
> > > > > > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > > > > > declaration can only be `ExternalLinkage` or `ExternalWeakLinkage`.
> > > > > > So I still believe we cannot set the resolver to
> > > > > > `LinkOnceODRLinkage/WeakODRLinkage` here, as they are declared but
> > > > > > not defined when we only have `cpu_specified` but no `cpu_dispatch`
> > > > > > in a TU as the example above.
> > > > > That doesn't seem right then. IF it allows ExternalWeakLinkage I'd
> > > > > expect WeakODR to work as well, since it is essentially the same
> > > > > thing.
> > > > I think we should have a double check. It is said "It is illegal for a
> > > > function declaration to have any linkage type other than `external` or
> > > > `extern_weak`" at the last line of section Linkage Type in the
> > > > reference manual [1]. I guess `weak_odr` is not designed for
> > > > declaration purpose and should be only used by definition.
> > > >
> > > > [1] https://llvm.org/docs/LangRef.html#linkage-types
> > > I had typed a reply, but apparently it didn't submit: Ah, nvm, I see now
> > > that external-weak is different from weak.
> > >
> > > I don't really get the linkages sufficiently to know what the right thing
> > > to do is then. If we DO have a definition, I'd say weak_odr so it can be
> > > merged, right? If we do NOT, could externally_available work?
> > No, I think it should be `external` instead of `available_externally`. The
> > later cannot used for declaration as well.
> >
> > So, getting back to the example, **1)** if we have `cpu_dispatch` and
> > `cpu_specific` in same TU, it's okay to use `weak_odr` for `foo.resolver`
> > as it is defined when `emitCPUDispatchDefinition` and it can be merged.
> > **2)** If we only have `cpu_specific` in a TU and have a reference to the
> > dispatched function, `foo.resolver` will be referenced without definition,
> > and `external` is the proper linkage to make it work.
> >
> > That's why I didn't set linkage type at this line.
> > No, I think it should be external instead of available_externally. The
> > later cannot used for declaration as well.
>
> Wouldn't that make it un-mergable later? Meaning, if you emitted the
> declaration from one TU, and the definition from another that you'd get a
> link error?
>
> I think the rules are more subtle than that. Any time you have a
> `cpu_dispatch`, the resolver is weak_odr so that it can be merged later. The
> presence of `cpu_specific`
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
zsrkmyn wrote:
> erichkeane wrote:
> > zsrkmyn wrote:
> > > erichkeane wrote:
> > > > zsrkmyn wrote:
> > > > > zsrkmyn wrote:
> > > > > > erichkeane wrote:
> > > > > > > zsrkmyn wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > This Resolver should have the same linkage as below.
> > > > > > > > Actually, I wanted to set linkage here at the first time, but
> > > > > > > > failed. When compiling code with cpu_specific but no
> > > > > > > > cpu_dispatch, we cannot set it as LinkOnceODR or WeakODR. E.g.:
> > > > > > > >
> > > > > > > > ```
> > > > > > > > $ cat specific_only.c
> > > > > > > > __declspec(cpu_specific(pentium_iii))
> > > > > > > > int foo(void) { return 0; }
> > > > > > > > int usage() { return foo(); }
> > > > > > > >
> > > > > > > > $ clang -fdeclspec specific_only.c
> > > > > > > >
> > > > > > > > Global is external, but doesn't have external or weak linkage!
> > > > > > > >
> > > > > > > > i32 ()* ()* @foo.resolver
> > > > > > > >
> > > > > > > > fatal error: error in backend: Broken module found, compilation
> > > > > > > > aborted!
> > > > > > > > ```
> > > > > > > >
> > > > > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in
> > > > > > > > which 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > > > > > > The crash message is complaining it isn't external/weak.
> > > > > > > However, WeakODR should count, right? Can you look into it a bit
> > > > > > > more to see what it thinks is broken?
> > > > > > No, actually I've tried it earlier with the example I mentioned in
> > > > > > my last comment, but WeakODR still makes compiler complaining. I
> > > > > > think it's `foo.resolver` that cannot be declared with as
> > > > > > WeakODR/LinkOnceODR without definition. But I'm really not familiar
> > > > > > with these rules.
> > > > > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > > > > declaration can only be `ExternalLinkage` or `ExternalWeakLinkage`.
> > > > > So I still believe we cannot set the resolver to
> > > > > `LinkOnceODRLinkage/WeakODRLinkage` here, as they are declared but
> > > > > not defined when we only have `cpu_specified` but no `cpu_dispatch`
> > > > > in a TU as the example above.
> > > > That doesn't seem right then. IF it allows ExternalWeakLinkage I'd
> > > > expect WeakODR to work as well, since it is essentially the same thing.
> > > I think we should have a double check. It is said "It is illegal for a
> > > function declaration to have any linkage type other than `external` or
> > > `extern_weak`" at the last line of section Linkage Type in the reference
> > > manual [1]. I guess `weak_odr` is not designed for declaration purpose
> > > and should be only used by definition.
> > >
> > > [1] https://llvm.org/docs/LangRef.html#linkage-types
> > I had typed a reply, but apparently it didn't submit: Ah, nvm, I see now
> > that external-weak is different from weak.
> >
> > I don't really get the linkages sufficiently to know what the right thing
> > to do is then. If we DO have a definition, I'd say weak_odr so it can be
> > merged, right? If we do NOT, could externally_available work?
> No, I think it should be `external` instead of `available_externally`. The
> later cannot used for declaration as well.
>
> So, getting back to the example, **1)** if we have `cpu_dispatch` and
> `cpu_specific` in same TU, it's okay to use `weak_odr` for `foo.resolver` as
> it is defined when `emitCPUDispatchDefinition` and it can be merged. **2)**
> If we only have `cpu_specific` in a TU and have a reference to the dispatched
> function, `foo.resolver` will be referenced without definition, and
> `external` is the proper linkage to make it work.
>
> That's why I didn't set linkage type at this line.
> No, I think it should be external instead of available_externally. The later
> cannot used for declaration as well.
Wouldn't that make it un-mergable later? Meaning, if you emitted the
declaration from one TU, and the definition from another that you'd get a link
error?
I think the rules are more subtle than that. Any time you have a
`cpu_dispatch`, the resolver is weak_odr so that it can be merged later. The
presence of `cpu_specific` shouldn't matter.
For 2, I think you're mostly correct, as long as the linker can still merge
them.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
erichkeane wrote:
> zsrkmyn wrote:
> > erichkeane wrote:
> > > zsrkmyn wrote:
> > > > zsrkmyn wrote:
> > > > > erichkeane wrote:
> > > > > > zsrkmyn wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > This Resolver should have the same linkage as below.
> > > > > > > Actually, I wanted to set linkage here at the first time, but
> > > > > > > failed. When compiling code with cpu_specific but no
> > > > > > > cpu_dispatch, we cannot set it as LinkOnceODR or WeakODR. E.g.:
> > > > > > >
> > > > > > > ```
> > > > > > > $ cat specific_only.c
> > > > > > > __declspec(cpu_specific(pentium_iii))
> > > > > > > int foo(void) { return 0; }
> > > > > > > int usage() { return foo(); }
> > > > > > >
> > > > > > > $ clang -fdeclspec specific_only.c
> > > > > > >
> > > > > > > Global is external, but doesn't have external or weak linkage!
> > > > > > >
> > > > > > > i32 ()* ()* @foo.resolver
> > > > > > >
> > > > > > > fatal error: error in backend: Broken module found, compilation
> > > > > > > aborted!
> > > > > > > ```
> > > > > > >
> > > > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in
> > > > > > > which 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > > > > > The crash message is complaining it isn't external/weak. However,
> > > > > > WeakODR should count, right? Can you look into it a bit more to
> > > > > > see what it thinks is broken?
> > > > > No, actually I've tried it earlier with the example I mentioned in my
> > > > > last comment, but WeakODR still makes compiler complaining. I think
> > > > > it's `foo.resolver` that cannot be declared with as
> > > > > WeakODR/LinkOnceODR without definition. But I'm really not familiar
> > > > > with these rules.
> > > > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > > > declaration can only be `ExternalLinkage` or `ExternalWeakLinkage`. So
> > > > I still believe we cannot set the resolver to
> > > > `LinkOnceODRLinkage/WeakODRLinkage` here, as they are declared but not
> > > > defined when we only have `cpu_specified` but no `cpu_dispatch` in a TU
> > > > as the example above.
> > > That doesn't seem right then. IF it allows ExternalWeakLinkage I'd
> > > expect WeakODR to work as well, since it is essentially the same thing.
> > I think we should have a double check. It is said "It is illegal for a
> > function declaration to have any linkage type other than `external` or
> > `extern_weak`" at the last line of section Linkage Type in the reference
> > manual [1]. I guess `weak_odr` is not designed for declaration purpose and
> > should be only used by definition.
> >
> > [1] https://llvm.org/docs/LangRef.html#linkage-types
> I had typed a reply, but apparently it didn't submit: Ah, nvm, I see now that
> external-weak is different from weak.
>
> I don't really get the linkages sufficiently to know what the right thing to
> do is then. If we DO have a definition, I'd say weak_odr so it can be
> merged, right? If we do NOT, could externally_available work?
No, I think it should be `external` instead of `available_externally`. The
later cannot used for declaration as well.
So, getting back to the example, **1)** if we have `cpu_dispatch` and
`cpu_specific` in same TU, it's okay to use `weak_odr` for `foo.resolver` as it
is defined when `emitCPUDispatchDefinition` and it can be merged. **2)** If we
only have `cpu_specific` in a TU and have a reference to the dispatched
function, `foo.resolver` will be referenced without definition, and `external`
is the proper linkage to make it work.
That's why I didn't set linkage type at this line.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
zsrkmyn wrote:
> erichkeane wrote:
> > zsrkmyn wrote:
> > > zsrkmyn wrote:
> > > > erichkeane wrote:
> > > > > zsrkmyn wrote:
> > > > > > erichkeane wrote:
> > > > > > > This Resolver should have the same linkage as below.
> > > > > > Actually, I wanted to set linkage here at the first time, but
> > > > > > failed. When compiling code with cpu_specific but no cpu_dispatch,
> > > > > > we cannot set it as LinkOnceODR or WeakODR. E.g.:
> > > > > >
> > > > > > ```
> > > > > > $ cat specific_only.c
> > > > > > __declspec(cpu_specific(pentium_iii))
> > > > > > int foo(void) { return 0; }
> > > > > > int usage() { return foo(); }
> > > > > >
> > > > > > $ clang -fdeclspec specific_only.c
> > > > > >
> > > > > > Global is external, but doesn't have external or weak linkage!
> > > > > >
> > > > > > i32 ()* ()* @foo.resolver
> > > > > >
> > > > > > fatal error: error in backend: Broken module found, compilation
> > > > > > aborted!
> > > > > > ```
> > > > > >
> > > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
> > > > > > 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > > > > The crash message is complaining it isn't external/weak. However,
> > > > > WeakODR should count, right? Can you look into it a bit more to see
> > > > > what it thinks is broken?
> > > > No, actually I've tried it earlier with the example I mentioned in my
> > > > last comment, but WeakODR still makes compiler complaining. I think
> > > > it's `foo.resolver` that cannot be declared with as WeakODR/LinkOnceODR
> > > > without definition. But I'm really not familiar with these rules.
> > > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > > declaration can only be `ExternalLinkage` or `ExternalWeakLinkage`. So I
> > > still believe we cannot set the resolver to
> > > `LinkOnceODRLinkage/WeakODRLinkage` here, as they are declared but not
> > > defined when we only have `cpu_specified` but no `cpu_dispatch` in a TU
> > > as the example above.
> > That doesn't seem right then. IF it allows ExternalWeakLinkage I'd expect
> > WeakODR to work as well, since it is essentially the same thing.
> I think we should have a double check. It is said "It is illegal for a
> function declaration to have any linkage type other than `external` or
> `extern_weak`" at the last line of section Linkage Type in the reference
> manual [1]. I guess `weak_odr` is not designed for declaration purpose and
> should be only used by definition.
>
> [1] https://llvm.org/docs/LangRef.html#linkage-types
I had typed a reply, but apparently it didn't submit: Ah, nvm, I see now that
external-weak is different from weak.
I don't really get the linkages sufficiently to know what the right thing to do
is then. If we DO have a definition, I'd say weak_odr so it can be merged,
right? If we do NOT, could externally_available work?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
erichkeane wrote:
> zsrkmyn wrote:
> > zsrkmyn wrote:
> > > erichkeane wrote:
> > > > zsrkmyn wrote:
> > > > > erichkeane wrote:
> > > > > > This Resolver should have the same linkage as below.
> > > > > Actually, I wanted to set linkage here at the first time, but failed.
> > > > > When compiling code with cpu_specific but no cpu_dispatch, we cannot
> > > > > set it as LinkOnceODR or WeakODR. E.g.:
> > > > >
> > > > > ```
> > > > > $ cat specific_only.c
> > > > > __declspec(cpu_specific(pentium_iii))
> > > > > int foo(void) { return 0; }
> > > > > int usage() { return foo(); }
> > > > >
> > > > > $ clang -fdeclspec specific_only.c
> > > > >
> > > > > Global is external, but doesn't have external or weak linkage!
> > > > >
> > > > > i32 ()* ()* @foo.resolver
> > > > >
> > > > > fatal error: error in backend: Broken module found, compilation
> > > > > aborted!
> > > > > ```
> > > > >
> > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
> > > > > 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > > > The crash message is complaining it isn't external/weak. However,
> > > > WeakODR should count, right? Can you look into it a bit more to see
> > > > what it thinks is broken?
> > > No, actually I've tried it earlier with the example I mentioned in my
> > > last comment, but WeakODR still makes compiler complaining. I think it's
> > > `foo.resolver` that cannot be declared with as WeakODR/LinkOnceODR
> > > without definition. But I'm really not familiar with these rules.
> > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > declaration can only be `ExternalLinkage` or `ExternalWeakLinkage`. So I
> > still believe we cannot set the resolver to
> > `LinkOnceODRLinkage/WeakODRLinkage` here, as they are declared but not
> > defined when we only have `cpu_specified` but no `cpu_dispatch` in a TU as
> > the example above.
> That doesn't seem right then. IF it allows ExternalWeakLinkage I'd expect
> WeakODR to work as well, since it is essentially the same thing.
I think we should have a double check. It is said "It is illegal for a function
declaration to have any linkage type other than `external` or `extern_weak`" at
the last line of section Linkage Type in the reference manual [1]. I guess
`weak_odr` is not designed for declaration purpose and should be only used by
definition.
[1] https://llvm.org/docs/LangRef.html#linkage-types
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
zsrkmyn wrote:
> zsrkmyn wrote:
> > erichkeane wrote:
> > > zsrkmyn wrote:
> > > > erichkeane wrote:
> > > > > This Resolver should have the same linkage as below.
> > > > Actually, I wanted to set linkage here at the first time, but failed.
> > > > When compiling code with cpu_specific but no cpu_dispatch, we cannot
> > > > set it as LinkOnceODR or WeakODR. E.g.:
> > > >
> > > > ```
> > > > $ cat specific_only.c
> > > > __declspec(cpu_specific(pentium_iii))
> > > > int foo(void) { return 0; }
> > > > int usage() { return foo(); }
> > > >
> > > > $ clang -fdeclspec specific_only.c
> > > >
> > > > Global is external, but doesn't have external or weak linkage!
> > > >
> > > > i32 ()* ()* @foo.resolver
> > > >
> > > > fatal error: error in backend: Broken module found, compilation
> > > > aborted!
> > > > ```
> > > >
> > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
> > > > 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > > The crash message is complaining it isn't external/weak. However,
> > > WeakODR should count, right? Can you look into it a bit more to see what
> > > it thinks is broken?
> > No, actually I've tried it earlier with the example I mentioned in my last
> > comment, but WeakODR still makes compiler complaining. I think it's
> > `foo.resolver` that cannot be declared with as WeakODR/LinkOnceODR without
> > definition. But I'm really not familiar with these rules.
> According to the `Verifier::visitGlobalValue()` in Verify.cpp, an declaration
> can only be `ExternalLinkage` or `ExternalWeakLinkage`. So I still believe we
> cannot set the resolver to `LinkOnceODRLinkage/WeakODRLinkage` here, as they
> are declared but not defined when we only have `cpu_specified` but no
> `cpu_dispatch` in a TU as the example above.
That doesn't seem right then. IF it allows ExternalWeakLinkage I'd expect
WeakODR to work as well, since it is essentially the same thing.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn marked an inline comment as done.
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
zsrkmyn wrote:
> erichkeane wrote:
> > zsrkmyn wrote:
> > > erichkeane wrote:
> > > > This Resolver should have the same linkage as below.
> > > Actually, I wanted to set linkage here at the first time, but failed.
> > > When compiling code with cpu_specific but no cpu_dispatch, we cannot set
> > > it as LinkOnceODR or WeakODR. E.g.:
> > >
> > > ```
> > > $ cat specific_only.c
> > > __declspec(cpu_specific(pentium_iii))
> > > int foo(void) { return 0; }
> > > int usage() { return foo(); }
> > >
> > > $ clang -fdeclspec specific_only.c
> > >
> > > Global is external, but doesn't have external or weak linkage!
> > >
> > > i32 ()* ()* @foo.resolver
> > >
> > > fatal error: error in backend: Broken module found, compilation aborted!
> > >
> > > ```
> > >
> > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
> > > 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > The crash message is complaining it isn't external/weak. However, WeakODR
> > should count, right? Can you look into it a bit more to see what it thinks
> > is broken?
> No, actually I've tried it earlier with the example I mentioned in my last
> comment, but WeakODR still makes compiler complaining. I think it's
> `foo.resolver` that cannot be declared with as WeakODR/LinkOnceODR without
> definition. But I'm really not familiar with these rules.
According to the `Verifier::visitGlobalValue()` in Verify.cpp, an declaration
can only be `ExternalLinkage` or `ExternalWeakLinkage`. So I still believe we
cannot set the resolver to `LinkOnceODRLinkage/WeakODRLinkage` here, as they
are declared but not defined when we only have `cpu_specified` but no
`cpu_dispatch` in a TU as the example above.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn marked an inline comment as done.
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
erichkeane wrote:
> zsrkmyn wrote:
> > erichkeane wrote:
> > > This Resolver should have the same linkage as below.
> > Actually, I wanted to set linkage here at the first time, but failed. When
> > compiling code with cpu_specific but no cpu_dispatch, we cannot set it as
> > LinkOnceODR or WeakODR. E.g.:
> >
> > ```
> > $ cat specific_only.c
> > __declspec(cpu_specific(pentium_iii))
> > int foo(void) { return 0; }
> > int usage() { return foo(); }
> >
> > $ clang -fdeclspec specific_only.c
> >
> > Global is external, but doesn't have external or weak linkage!
> >
> > i32 ()* ()* @foo.resolver
> >
> > fatal error: error in backend: Broken module found, compilation aborted!
> > ```
> >
> > This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
> > 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> The crash message is complaining it isn't external/weak. However, WeakODR
> should count, right? Can you look into it a bit more to see what it thinks
> is broken?
No, actually I've tried it earlier with the example I mentioned in my last
comment, but WeakODR still makes compiler complaining. I think it's
`foo.resolver` that cannot be declared with as WeakODR/LinkOnceODR without
definition. But I'm really not familiar with these rules.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
zsrkmyn wrote:
> erichkeane wrote:
> > This Resolver should have the same linkage as below.
> Actually, I wanted to set linkage here at the first time, but failed. When
> compiling code with cpu_specific but no cpu_dispatch, we cannot set it as
> LinkOnceODR or WeakODR. E.g.:
>
> ```
> $ cat specific_only.c
> __declspec(cpu_specific(pentium_iii))
> int foo(void) { return 0; }
> int usage() { return foo(); }
>
> $ clang -fdeclspec specific_only.c
>
> Global is external, but doesn't have external or weak linkage!
>
> i32 ()* ()* @foo.resolver
>
> fatal error: error in backend: Broken module found, compilation aborted!
> ```
>
> This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
> 'SingleVersion()' doesn't have a cpu_dispatch declaration.
The crash message is complaining it isn't external/weak. However, WeakODR
should count, right? Can you look into it a bit more to see what it thinks is
broken?
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3005
/*ForVTable=*/false);
+auto Linkage = (FD->isCPUDispatchMultiVersion() ||
FD->isCPUSpecificMultiVersion())
+? llvm::Function::LinkOnceODRLinkage
zsrkmyn wrote:
> erichkeane wrote:
> > I think this can always just be LinkOnceODR.
> Changing this also changes linkage for attribute(target()), should I also
> change test cases for them? (including test/CodeGen{,CXX}/attr-*.ll)
Yep! Also, I think WeakODR is the right one.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn marked an inline comment as done.
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3005
/*ForVTable=*/false);
+auto Linkage = (FD->isCPUDispatchMultiVersion() ||
FD->isCPUSpecificMultiVersion())
+? llvm::Function::LinkOnceODRLinkage
erichkeane wrote:
> I think this can always just be LinkOnceODR.
Changing this also changes linkage for attribute(target()), should I also
change test cases for them? (including test/CodeGen{,CXX}/attr-*.ll)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
erichkeane wrote:
> This Resolver should have the same linkage as below.
Actually, I wanted to set linkage here at the first time, but failed. When
compiling code with cpu_specific but no cpu_dispatch, we cannot set it as
LinkOnceODR or WeakODR. E.g.:
```
$ cat specific_only.c
__declspec(cpu_specific(pentium_iii))
int foo(void) { return 0; }
int usage() { return foo(); }
$ clang -fdeclspec specific_only.c
Global is external, but doesn't have external or weak linkage!
i32 ()* ()* @foo.resolver
fatal error: error in backend: Broken module found, compilation aborted!
```
This is found by lit test test/CodeGen/attr-cpuspecific.c, in which
'SingleVersion()' doesn't have a cpu_dispatch declaration.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added a comment. Actually... I think it might need to be weak_odr based on https://llvm.org/docs/LangRef.html#linkage-types We want the merge semantics, but need to make sure that the symbols aren't discarded. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67058/new/ https://reviews.llvm.org/D67058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
erichkeane added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
This Resolver should have the same linkage as below.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3005
/*ForVTable=*/false);
+auto Linkage = (FD->isCPUDispatchMultiVersion() ||
FD->isCPUSpecificMultiVersion())
+? llvm::Function::LinkOnceODRLinkage
I think this can always just be LinkOnceODR.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc & Fix resolver linkage type
zsrkmyn updated this revision to Diff 218830.
zsrkmyn retitled this revision from "[clang][CodeGen] Add alias for
cpu_dispatch function with IFunc" to "[clang][CodeGen] Add alias for
cpu_dispatch function with IFunc & Fix resolver linkage type".
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
Files:
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGen/attr-cpuspecific.c
clang/test/CodeGenCXX/attr-cpuspecific.cpp
Index: clang/test/CodeGenCXX/attr-cpuspecific.cpp
===
--- clang/test/CodeGenCXX/attr-cpuspecific.cpp
+++ clang/test/CodeGenCXX/attr-cpuspecific.cpp
@@ -13,13 +13,15 @@
s.Func();
}
-// LINUX: define void (%struct.S*)* @_ZN1S4FuncEv.resolver
+// LINUX: @_ZN1S4FuncEv = linkonce_odr alias void (%struct.S*), void (%struct.S*)* @_ZN1S4FuncEv.ifunc
+// LINUX: @_ZN1S4FuncEv.ifunc = linkonce_odr ifunc void (%struct.S*), void (%struct.S*)* ()* @_ZN1S4FuncEv.resolver
+// LINUX: define linkonce_odr void (%struct.S*)* @_ZN1S4FuncEv.resolver
// LINUX: ret void (%struct.S*)* @_ZN1S4FuncEv.S
// LINUX: ret void (%struct.S*)* @_ZN1S4FuncEv.O
// LINUX: declare void @_ZN1S4FuncEv.S
// LINUX: define linkonce_odr void @_ZN1S4FuncEv.O
-// WINDOWS: define dso_local void @"?Func@S@@QEAAXXZ"(%struct.S* %0)
+// WINDOWS: define linkonce_odr dso_local void @"?Func@S@@QEAAXXZ"(%struct.S* %0) comdat
// WINDOWS: musttail call void @"?Func@S@@QEAAXXZ.S"(%struct.S* %0)
// WINDOWS: musttail call void @"?Func@S@@QEAAXXZ.O"(%struct.S* %0)
// WINDOWS: declare dso_local void @"?Func@S@@QEAAXXZ.S"
Index: clang/test/CodeGen/attr-cpuspecific.c
===
--- clang/test/CodeGen/attr-cpuspecific.c
+++ clang/test/CodeGen/attr-cpuspecific.c
@@ -7,11 +7,27 @@
#define ATTR(X) __attribute__((X))
#endif // _MSC_VER
-// Each called version should have an IFunc.
-// LINUX: @SingleVersion.ifunc = ifunc void (), void ()* ()* @SingleVersion.resolver
-// LINUX: @TwoVersions.ifunc = ifunc void (), void ()* ()* @TwoVersions.resolver
-// LINUX: @TwoVersionsSameAttr.ifunc = ifunc void (), void ()* ()* @TwoVersionsSameAttr.resolver
-// LINUX: @ThreeVersionsSameAttr.ifunc = ifunc void (), void ()* ()* @ThreeVersionsSameAttr.resolver
+// Each version should have an IFunc and an alias.
+// LINUX: @TwoVersions = linkonce_odr alias void (), void ()* @TwoVersions.ifunc
+// LINUX: @TwoVersionsSameAttr = linkonce_odr alias void (), void ()* @TwoVersionsSameAttr.ifunc
+// LINUX: @ThreeVersionsSameAttr = linkonce_odr alias void (), void ()* @ThreeVersionsSameAttr.ifunc
+// LINUX: @NoSpecifics = linkonce_odr alias void (), void ()* @NoSpecifics.ifunc
+// LINUX: @HasGeneric = linkonce_odr alias void (), void ()* @HasGeneric.ifunc
+// LINUX: @HasParams = linkonce_odr alias void (i32, double), void (i32, double)* @HasParams.ifunc
+// LINUX: @HasParamsAndReturn = linkonce_odr alias i32 (i32, double), i32 (i32, double)* @HasParamsAndReturn.ifunc
+// LINUX: @GenericAndPentium = linkonce_odr alias i32 (i32, double), i32 (i32, double)* @GenericAndPentium.ifunc
+// LINUX: @DispatchFirst = linkonce_odr alias i32 (), i32 ()* @DispatchFirst.ifunc
+
+// LINUX: @TwoVersions.ifunc = linkonce_odr ifunc void (), void ()* ()* @TwoVersions.resolver
+// LINUX: @SingleVersion.ifunc = linkonce_odr ifunc void (), void ()* ()* @SingleVersion.resolver
+// LINUX: @TwoVersionsSameAttr.ifunc = linkonce_odr ifunc void (), void ()* ()* @TwoVersionsSameAttr.resolver
+// LINUX: @ThreeVersionsSameAttr.ifunc = linkonce_odr ifunc void (), void ()* ()* @ThreeVersionsSameAttr.resolver
+// LINUX: @NoSpecifics.ifunc = linkonce_odr ifunc void (), void ()* ()* @NoSpecifics.resolver
+// LINUX: @HasGeneric.ifunc = linkonce_odr ifunc void (), void ()* ()* @HasGeneric.resolver
+// LINUX: @HasParams.ifunc = linkonce_odr ifunc void (i32, double), void (i32, double)* ()* @HasParams.resolver
+// LINUX: @HasParamsAndReturn.ifunc = linkonce_odr ifunc i32 (i32, double), i32 (i32, double)* ()* @HasParamsAndReturn.resolver
+// LINUX: @GenericAndPentium.ifunc = linkonce_odr ifunc i32 (i32, double), i32 (i32, double)* ()* @GenericAndPentium.resolver
+// LINUX: @DispatchFirst.ifunc = linkonce_odr ifunc i32 (), i32 ()* ()* @DispatchFirst.resolver
ATTR(cpu_specific(ivybridge))
void SingleVersion(void){}
@@ -29,14 +45,14 @@
ATTR(cpu_dispatch(ivybridge, knl))
void TwoVersions(void);
-// LINUX: define void ()* @TwoVersions.resolver()
+// LINUX: define linkonce_odr void ()* @TwoVersions.resolver()
// LINUX: call void @__cpu_indicator_init
// LINUX: ret void ()* @TwoVersions.Z
// LINUX: ret void ()* @TwoVersions.S
// LINUX: call void @llvm.trap
// LINUX: unreachable
-// WINDOWS: define dso_local void @TwoVersions()
+// WINDOWS: define linkonce_odr dso_local void @TwoVersions() comdat
// WINDOWS: call void @__cpu_indicator_init()
// WINDOWS: call void @TwoVersions.Z()
// WINDOWS-NEXT: ret void
@@ -82,14 +98,14 @@
