[PATCH] D157750: Properly handle -fsplit-machine-functions for fatbinary compilation

[Han Shen via Phabricator via cfe-commits]

shenhan added inline comments.


================
Comment at: clang/test/Driver/fsplit-machine-functions-with-cuda-nvptx.c:9
+
+// Check that -fsplit-machine-functions is passed to both x86 and cuda compilation and does not cause driver error.
+// MFS2: -fsplit-machine-functions
----------------
tra wrote:
> shenhan wrote:
> > tra wrote:
> > > We will still see a warning, right? So, for someone compiling with `-Werror` that's going to be a problem.
> > > 
> > > Also, if the warning is issued from the top-level driver, we may not even be able to suppress it when we disable splitting on GPU side with `-Xarch_device -fno-split-machine-functions`.
> > > 
> > > 
> > > We will still see a warning, right?
> > Yes, there still will be a warning. We've discussed it and we think that pass -fsplit-machine-functions in this case is not a proper usage and a warning is warranted, and it is not good that skip doing split silently while uses explicitly ask for it.
> > 
> > > Also, if the warning is issued from the top-level driver
> > The warning will not be issued from the top-level driver, it will be issued when configuring optimization passes.
> > So:
> > 
> > 
> >   - -fsplit-machine-functions -Xarch_device -fno-split-machine-functions
> > Will enable MFS for host, disable MFS for gpus and without any warnings.
> > 
> >   - -Xarch_host -fsplit-machine-functions
> > The same as the above
> > 
> >   - -Xarch_host -fsplit-machine-functions -Xarch_device -fno-split-machine-functions
> > The same as the above
> > 
> > We've discussed it and we think that pass -fsplit-machine-functions in this case is not a proper usage and a warning is warranted, and it is not good that skip doing split silently while uses explicitly ask for it.
> 
> I would agree with that assertion if we were talking exclusively about CUDA compilation.
> However, a common real world use pattern is that the flags are set globally for all C++ compilations, and then CUDA compilations within the project need to do whatever they need to to keep things working. The original user intent was for the option to affect the host compilation. There's no inherent assumption that it will do anything useful for the GPU.
> 
> In number of similar cases in the past we did settle on silently ignoring some top-level flags that we do expect to encounter in real projects, but which made no sense for the GPU. E.g. sanitizers. If the project is built w/ sanitizer enabled, the idea is to sanitize the host code, The GPU code continues to be built w/o sanitizer enabled. 
> 
> Anyways, as long as we have a way to deal with it it's not a big deal one way or another.
> 
> > -fsplit-machine-functions -Xarch_device -fno-split-machine-functions
> > Will enable MFS for host, disable MFS for gpus and without any warnings.
> 
> OK. This will work.
> 
> 
> In number of similar cases in the past we did settle on silently ignoring some top-level flags that we do expect to encounter in real projects, but which made no sense for the GPU. E.g. sanitizers. If the project is built w/ sanitizer enabled, the idea is to sanitize the host code, The GPU code continues to be built w/o sanitizer enabled.

Can I understand it this way - if the compiler is **only** building for CPUs, then silently ignore any optimization flags is not a good behavior. If the compiler is building CPUs and GPUs, it is still not a good behavior to silently ignore optimization flags for CPUs, but it is probably ok to silently ignore optimization flags for GPUs.

> OK. This will work.
Thanks for confirming.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1281-1282
+    else
+      WithColor::warning()
+          << "-fsplit-machine-functions is only valid for X86.\n";
   }
----------------
arsenm wrote:
> You cannot spam warnings here. The other instance of printing here looks like a new addition and should be removed
Thanks. Do you suggest moving the warnings to the underlying pass? (Although that means we create passes that only issue warnings.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157750