[PATCH] D155856: [HIP][LLVM][Opt][RFC] Add LLVM support for C++ Parallel Algorithm Offload

[Alex Voicu via Phabricator via llvm-commits]

AlexVlx added a comment.

In D155856#4644379 <https://reviews.llvm.org/D155856#4644379>, @yaxunl wrote:

> Are these passes called when compiling C++ to bitcode? If so, then device functions not called by kernels in the same module will be removed, right? Then we cannot support calling device functions in a different TU. Should these passes be moved to the llvm codegen pipelines so that they will only be called post-linking?

The selection pass is run post-linking, please see https://reviews.llvm.org/D155775, more specifically the changes done to how we form the `lld` invocation. The allocator interposition stuff is run / driven by Clang, however it is not predicated on having the full module available / running after linking, and wouldn't benefit from it.



================
Comment at: llvm/lib/Transforms/HipStdPar/HipStdPar.cpp:62
+template<typename T>
+static inline void eraseFromModule(T &toErase) {
+  toErase.replaceAllUsesWith(PoisonValue::get(toErase.getType()));
----------------
yaxunl wrote:
> variable names should be capitalized https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
Will fix, thanks.


================
Comment at: llvm/lib/Transforms/HipStdPar/HipStdPar.cpp:73
+
+  if (!G.isConstantUsed())
+    return true;
----------------
yaxunl wrote:
> Is this condition correct?
> 
> AMDGPU backend supports non-constant TLS?
Typo, will fix, good catch!


================
Comment at: llvm/lib/Transforms/HipStdPar/HipStdPar.cpp:135
+
+    G.setLinkage(GlobalVariable::ExternalWeakLinkage);
+    G.setExternallyInitialized(true);
----------------
yaxunl wrote:
> Any reason to change the linkage to weak?
Yes, this is a precursor to adding actual support for globals, which will entail binding them. Since at the moment they are not handled (will require a RT addition), we're giving them weak linkage toe eschew the need to define (bind) them at code object load time, otherwise for declarations this'd lead to a load time error. The other option would be to turn all declarations into definitions, with the initialiser being poison (but then this'd need to be undone when we add support).


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

https://reviews.llvm.org/D155856