[PATCH] D137470: [Offloading] Initial support for registering offloading entries on COFF targets

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This looks reasonable to me, overall. I didn't quite try to follow all the wall-of-text changes in the tests though, but overall fine. Just a couple comments and one case where I didn't find where code went in the refactoring.



================
Comment at: clang/test/CodeGenCUDA/offloading-entries.cu:92
+// CUDA-COFF-NEXT:  entry:
+// CUDA-COFF-NEXT:    [[TMP0:%.*]] = call i32 @cudaLaunch(ptr @_Z18__device_stub__barv)
+// CUDA-COFF-NEXT:    br label [[SETUP_END:%.*]]
----------------
Is this identical to the one above? Should the lines be shared with `--check-prefix=COMMON,COFF` etc? (The number of lines is rather small here so it's maybe not strictly necessary, but I saw that done in the other testcase.)


================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:145
+  // For COFF targets, sections with 8 or fewer characters containing a '$' will
+  // be merged into the same section at runtime. The order is determined by the
+  // alphebetical ordering of the text after the '$' character. Here we generate
----------------
FWIW, this comment doesn't feel entirely accurate: Regardless of the length of the section name, all sections with names of the form `name$suffix` will get merged into the same section `name` (sorted by the suffix). Then if `name` is 8 chars or less, the name is kept in the section table (so that it can easily be looked up at runtime), while if it is longer, the full name is kept in the string table (which is not mapped at runtime).

Also as an extra side note; we added an exception into lld for `.eh_frame` - this is 9 chars, but libunwind wants to locate the section at runtime. So for that case, lld truncates it to `.eh_fram`. (This behaviour is lld specific, to appease libunwind - binutils doesn't do that, and libgcc locates that section differently.)


================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:337
-      M, DummyInit->getType(), true, GlobalVariable::ExternalLinkage, DummyInit,
-      IsHIP ? "__dummy.hip_offloading.entry" : "__dummy.cuda_offloading.entry");
-  DummyEntry->setVisibility(GlobalValue::HiddenVisibility);
----------------
I don't quite see where the corresponding GlobalVariable for this case is created after the refactoring?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137470/new/

https://reviews.llvm.org/D137470