[PATCH] D80750: llvm-link: Add module flag behavior MergeTargetID

Mon Jul 27 23:06:10 PDT 2020

jdoerfert added inline comments.

================
Comment at: llvm/docs/LangRef.rst:6493
+           is in both source and destination target ID's but with different
+           signs, an error of conflict module flags will be emitted.
+
----------------
yaxunl wrote:
> jdoerfert wrote:
> > yaxunl wrote:
> > > jdoerfert wrote:
> > > > This sounds an awful lot like mismatches in regular target triples. Do we really need a new mechanism and wording here, and if so, couldn't we restrict it to the features? I mean, there is a target triple already in the module, right?
> > > Yes. It is a design decision made by AMD after thorough internal discussions.
> > > 
> > > We need to have an efficient way to identify device binaries embedded in a host executable for single source languages e.g. HIP. There are multiple device binaries embedded. The device binaries are not just per processor, they are per processor/feature combination. We do not want to encode features in GPU names since it incurs combination explosion. In stead, we need to use processor:feature1+:feature2+ (so called target ID) to identify a device binary.
> > > 
> > > The target ID is a real ID to identify a device binary. It is specified by user to clang and will be passed to backend to embed in device binary to be used by runtime. Since it is per module, it needs to be represented as a module flag. And since modules with different target ID may be linked together, they need to be checked to ensure compatibility and merged if necessary.
> > > 
> > > Checking target feature directly is not suitable here since target feature is per function. Also not all target features are part of target ID.
> > > 
> > > Currently target ID is only supported by AMDGPU target. It is NFC for other targets. However it can be adopted by other targets easily. 
> > > 
> > > Yes. It is a design decision made by AMD after thorough internal discussions.
> > 
> > Given that this is a problem for various people and languages, maybe such a discussion should happen in the open such that we implement a solution which can be used by HIP, OPENMP, CUDA, SYCL, ...
> > 
> > As an alternative design, I have (for a while now) a prototype to allow multi-target LLVM-IR modules. It seems that would solve your problem as well but (IMHO) closer aligned to what we use "usually" to define the target (namely the target string in the module).
> > 
> > I'll share my prototype this week and start a discussion. Feel free to let me know beforehand what you think.
> Have you shared your prototype? We would like to evaluate using it in place of the module flag. Thanks.
Sorry for the wait, this was not good of me.

I wrote the email I was postponing for months: http://lists.llvm.org/pipermail/llvm-dev/2020-July/143808.html
And you can see one way of having different targets in one module prototyped in D84728.

Please let me know what you think.

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

https://reviews.llvm.org/D80750