jhuber6 marked an inline comment as done.
jhuber6 added a comment.
In D142484#4078377 <https://reviews.llvm.org/D142484#4078377>, @tra wrote:
> LGTM. Please wait a bit before landing it, in case @MaskRay has something to
> say.
I'm somewhat hoping to get this in before the fork that happens in a few hours.
================
Comment at: clang/test/Driver/linker-wrapper-libs.c:38-39
+
+// LIBRARY-RESOLVES: clang{{.*}} -o {{.*}}.img --target=amdgcn-amd-amdhsa
-mcpu=gfx1030 {{.*}}.o {{.*}}.o
+// LIBRARY-RESOLVES: clang{{.*}} -o {{.*}}.img --target=nvptx64-nvidia-cuda
-march=sm_70 {{.*}}.s {{.*}}.o
+
----------------
tra wrote:
> I'm puzzled about how exactly we're checking the `that we extract a static
> library defining an undefined symbol` part here.
>
> Presumably the trailing .o would be the object extracted from the library.
> I'm not sure where the first .o/.s argument come from? Is that the 'main' GPU
> object/executable embedded in the host object?
>
Yes, if we extract it we'll have two object files. The `.s` comes from the fact
that I deliberately made the main binary an ELF while the library is a `.bc`.
This is why we get a `.s` for Nvidia, it's PTX.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1213
+
+ // We will extract if it defines a new global symbol visible to the host.
+ bool NewGlobalSymbol = ((NewSymbol || (OldSym & Sym_Undefined)) &&
----------------
tra wrote:
> Nit: perhaps we should rename `Resolved` -> `ShouldExtractObject`, it would
> be a better match. After all, defining/including a new symbol is not quite
> the same as resolving a reference.
Good point, I'll change it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142484/new/
https://reviews.llvm.org/D142484
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
