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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

Thanks for the feedback.

Another significant portion of getting this workflow to work for Windows / COFF is parsing the linker arguments. I should be able to look at `lld-link` and add necessarily aliases to what `ld.lld` takes I assume? E.g. we use values like `-o` and `-L` in the linker wrapper to set the output and find libraries.



================
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:%.*]]
----------------
mstorsjo wrote:
> 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.)
Yeah it might be valid to collapse it further, this test is mostly just copy-pasted directly from the output so we should probably try to keep it common.


================
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
----------------
mstorsjo wrote:
> 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.)
I see, I'm not that familiar with the inner workings of the COFF linking process. All that matters for this use-case is whether or not we can get a pointer to the array. In that case we shouldn't need to worry about the eight character limit right?


================
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);
----------------
mstorsjo wrote:
> I don't quite see where the corresponding GlobalVariable for this case is created after the refactoring?
The CUDA / HIP cases did this separately. This patch merged it into a common method `getELFEntriesArray`. Functionally this just changed the order in the output slightly. The `dummy` variable is only necessary for the ELF linkers to generate the begin / end section. For COFF we make the `$a` and `$z` variables which perform a similar role.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137470