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` shouldn't matter. > > For 2, I think you're mostly correct, as long as the linker can still merge > them. > Wouldn't that make it un-mergable later? No, it wouldn't. Declaration has nothing to do with definition IMO. ``` $ cat main.ll declare external i32 @bar() define i32 @main() { %call = call i32 @bar() ret i32 %call } $ cat bar.ll define weak_odr i32 @bar() { ret i32 10 } $ cp bar.ll bar2.ll # copy here so we have 2 weak 'bar' $ clang -c main.ll bar.ll bar2.ll $ clang main.o bar.o bar2.o -o main $ ./main $ echo $? 10 $ nm bar.o 0000000000000000 W bar $ nm main.o U bar 0000000000000000 T main ``` > 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. Yes, you're right. So setting linkage type at line 3005 does it: if we have a cpu_dispatch, then it's set to `weak_odr`. 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