[PATCH] D68587: [hip] Assume host-only compilation if the final phase is ahead of `backend`.

Mon Oct 7 18:41:43 PDT 2019

hliao added a comment.

In D68587#1698247 <https://reviews.llvm.org/D68587#1698247>, @tra wrote:

> In D68587#1698102 <https://reviews.llvm.org/D68587#1698102>, @hliao wrote:
>
> > for most compilation tools, single input and single output are expected. Without assuming `-fsyntax-only` alone is host-compilation only, that at least run syntax checking twice.
>
>
> I believe the driver will not run subsequent jobs if one of the device compilations fails. You may see duplicate warnings from multiple stages, but overall the error handling works in a fairly predictable way now.

It still runs and gives the same error (if that error is applicable to both sides) at least twice if you just specify `-fsyntax-only` or `-E`. That won't happen for regular compilation option (`-c`) due to the additional device dependencies added.
The error itself is, in fact, should be clear enough, the most confusing part is the diagnostic message and suggestions from clang as host- and device-side compilations are quite different, especially the error message may be mixed with other-side the normal output.

> 
> 
>> The result may be misleading
> 
> Potentially repeated warning are still *correct*, while omitting an error is not, IMO.  I believe that did come up before and we had to make some changes to the driver to keep host compilation working even when device-side compilations produce no output.
> 
> To think of it, I'm starting to doubt that this patch is an improvement for `-M` either. You will get the dependencies for the host, but they are not necessarily the same as the dependencies for the device-side compilation. Producing a partial list of dependencies will be potentially incorrect. IMO we do need dependencies info from all sub-compilations.

Even without this patch, `-M` or more specifically `-MD` already breaks now as we just run the dependency generation action twice for each side. The later will overwrite the former *.d file. We need special handling of `-M` to match nvcc.

> Perhaps we should limit the scope of this patch to -E only for now?

Just found nvcc's `-E` returns the output of the device-side compilation for the first GPU arch specified. Anyway, whether to match that behavior is just another question.

> 
> 
>> and there are clang-based tools (like clang-tidy) may have no legacy way to be runnable.
> 
> Tooling does get surprised by the multiple jobs created by CUDA compilation. 
>  The work around is to pass `--cuda-host-only`. Passing an extra flag is usually not a showstopper (at least it was not in cases I had to deal with at work and we have fair number of clang-derived tools). Usually in order to get correct CUDA compilation in this scenario you will also need to tell the tool where to find CUDA's headers, so the mechanism for passing additional options already exists.

but some tools, like clang-tidy, may be found difficult to insert that option properly, says `clang-tidy -p` supposes to read the compilation command databased generated by cmake or meta-build systems and performs additional checks for sources. Adding that option may inevitably make them CUDA/HIP aware.

> If that's still a problem, then we should change the tooling infrastructure to use host-only compilation for HIP and CUDA by default.

That's an option I was looking into as well. But, generally speaking, we need a clear definition on expected output for options like `-M`, `-MD`, `-E`, `-fsyntax-only`, `-S -emit-llvm`, even `-S` (for HIP only.)

>> To check device-side compilation syntax, we are still able to explicitly ask that by specifying `--cuda-device-only`.
> 
> Yes, that could be done. However, as I've mentioned above the same argument applies to tooling & `--cuda-host-only`, so there's no advantage here. IMO the most common use case should be the default, so it's the clang itself which should remain working correctly without having to add extra flags.
> 
> Also, this patch makes it impossible to run -fsyntax-only on *all* sub-compilations at once. I will have to run `clang -fsyntax-only` multiple times -- once per host and once per each device.

We do have another option `-cuda-compile-host-device` to explicit ask for host- and device-side compilations.

> I do want to check all sub-compilations with `-fsyntax-only` on a regular basis (e.g. when running creduce on a cuda source), so having to do that for each sub-compilation separately does not look like an improvement to me.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68587