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?
> >
> > 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`.
I see, thank you for clarifying.
These changes should likely be reflected for target MV as well, but in that
case the resolver should likely always be weak_odr linkage (since it is always
emitted).
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
