tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
LGTM. Please wait a bit before landing it, in case @MaskRay has something to 
say.



================
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
+
----------------
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?



================
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)) &&
----------------
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.


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

Reply via email to