This is an archive of the discontinued LLVM Phabricator instance.

[clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading
ClosedPublic

Authored by mhalk on Mar 8 2023, 9:07 AM.

Details

Summary

Adds a warning, issued by the clang semantic analysis, if HIP and OpenMP target offloading is requested concurrently.
That is, if HIP language mode is active but OpenMP target directives are encountered.
Previously, a user might not have been aware that target directives are ignored in such a case.

Generation of this warning is (lit-)tested via "make check-clang-semaopenmp".
The warning can be ignored via "-Wno-hip-omp-target-directives".

Diff Detail

Event Timeline

mhalk created this revision.Mar 8 2023, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 9:07 AM
mhalk requested review of this revision.Mar 8 2023, 9:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
mjklemm added a subscriber: mjklemm.Mar 8 2023, 9:33 AM
scchan added a subscriber: scchan.

LGTM

yaxunl added a reviewer: tra.Mar 8 2023, 12:47 PM
tra added a comment.Mar 8 2023, 1:11 PM

How is this different from compiling a C++ file with opemnp directives in it? AFAICT neither clang nor gcc issue anywarnings: https://godbolt.org/z/5Me3dnTdr

What makes the warnings necessary for HIP?

mhalk added a comment.EditedMar 9 2023, 1:56 AM

How is this different from compiling a C++ file with opemnp directives in it? AFAICT neither clang nor gcc issue anywarnings: https://godbolt.org/z/5Me3dnTdr

What makes the warnings necessary for HIP?

When using HIP target offloading it will "take precedence" over OpenMP target offloading.
That is, there is a use case which compiles "successfully" / without warning but will not execute code within OpenMP target directives.

For example: when using -x hip -fopenmp --offload-arch=... (+ other reasonable parameters) a HIP program with OpenMP target directives will compile without warning / error.
But only host-related OpenMP code will be executed properly.

Another hint that HIP and OpenMP target offloading do not work concurrently ATM is that when compiling with -x hip -fopenmp -fopenmp-targets=... clang will throw an error.

Hence, we want to signal this issue to users, whorun into problems and then have to debug their programs to find out about this.
AFAIK this issue can be alleviated by refactoring / splitting up your code, then linking it back togehter -- but you have to know about this.

tra added a subscriber: jhuber6.Mar 9 2023, 10:07 AM

For example: when using -x hip -fopenmp --offload-arch=... (+ other reasonable parameters) a HIP program with OpenMP target directives will compile without warning / error.
But only host-related OpenMP code will be executed properly.

Another hint that HIP and OpenMP target offloading do not work concurrently ATM is that when compiling with -x hip -fopenmp -fopenmp-targets=... clang will throw an error.

It sounds like what we want is to make -x hip and -fopenmp mutually exclusive, with a hard error when both are used. If you look at the problem as "-fopenmp completely breaks HIP compilation", a warning is a bit too weak of a measure, IMO.

We can restate it all as "mixing offloading modes is not supported" and generalize it to both CUDA and HIP.

@jhuber6 - WDYT?

It sounds like what we want is to make -x hip and -fopenmp mutually exclusive, with a hard error when both are used. If you look at the problem as "-fopenmp completely breaks HIP compilation", a warning is a bit too weak of a measure, IMO.

We can restate it all as "mixing offloading modes is not supported" and generalize it to both CUDA and HIP.

-x hip and -fopenmp has been a valid combination. -fopenmp with -x hip allows non-offloading OpenMP directives in host code in HIP. It just ignores the offloading directives.

tra added a comment.Mar 9 2023, 11:37 AM

-x hip and -fopenmp has been a valid combination. -fopenmp with -x hip allows non-offloading OpenMP directives in host code in HIP. It just ignores the offloading directives.

That brings me back to the earlier question -- what do we currently do when target directives are encountered when we compile a C++ source w/ OpenMP enabled and why HIP shold be handled differently.

If a warning makes sense for target directives with offloading disabled, that warning would be equally applicable to C/C++/CUDA & HIP. If that's not the case, what am I missing?

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

jdoerfert added inline comments.Mar 9 2023, 12:42 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

I doubt the ignored part very much.

yaxunl added a comment.Mar 9 2023, 1:20 PM

-x hip and -fopenmp has been a valid combination. -fopenmp with -x hip allows non-offloading OpenMP directives in host code in HIP. It just ignores the offloading directives.

That brings me back to the earlier question -- what do we currently do when target directives are encountered when we compile a C++ source w/ OpenMP enabled and why HIP shold be handled differently.

If a warning makes sense for target directives with offloading disabled, that warning would be equally applicable to C/C++/CUDA & HIP. If that's not the case, what am I missing?

OpenMP offloading directives (e.g. "omp target") create implicit GPU kernels which require OpenMP toolchain to create offloading actions to support them. For C/C++ programs, OpenMP toolchain can create offloading actions to support the OpenMP offloading directives. For HIP programs, HIP toolchain can only do offloading for HIP, not for OpenMP, therefore the OpenMP offloading directives are ignored. However, the other OpenMP directives e.g. "omp parallel for" works on CPU without offloading toolchain support. Currently, the clang driver and OpenMP/HIP runtime are not ready to support both HIP and OpenMP offloading at the same time.

yaxunl added a comment.Mar 9 2023, 1:23 PM

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

We cannot filter out -fopenmp for HIP job because the host code in HIP program needs it to support "omp parallel for" etc. Filtering it will break existing HIP programs.

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

We cannot filter out -fopenmp for HIP job because the host code in HIP program needs it to support "omp parallel for" etc. Filtering it will break existing HIP programs.

I mean, couldn't we just prevent the -cc1 arguments for the HIP device compilation from using any OpenMP? Or is that breaking. I figured it was only supported for the CPU portion.

yaxunl added a comment.Mar 9 2023, 1:33 PM

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

We cannot filter out -fopenmp for HIP job because the host code in HIP program needs it to support "omp parallel for" etc. Filtering it will break existing HIP programs.

I mean, couldn't we just prevent the -cc1 arguments for the HIP device compilation from using any OpenMP? Or is that breaking. I figured it was only supported for the CPU portion.

We tried that before but it won't work since that will make users' code inconsistent for host/device compilation and cause all kinds of ODR violations.

tra added a comment.Mar 9 2023, 1:56 PM

OpenMP offloading directives (e.g. "omp target") create implicit GPU kernels which require OpenMP toolchain to create offloading actions to support them. For C/C++ programs, OpenMP toolchain can create offloading actions to support the OpenMP offloading directives. For HIP programs, HIP toolchain can only do offloading for HIP, not for OpenMP, therefore the OpenMP offloading directives are ignored. However, the other OpenMP directives e.g. "omp parallel for" works on CPU without offloading toolchain support. Currently, the clang driver and OpenMP/HIP runtime are not ready to support both HIP and OpenMP offloading at the same time.

So, we essentially end up with GPU code intended to be used with different runtimes, and we only support one kind at a time. OK. I think I'm getting a rough idea of what's going on.

Ignoring target openmp directives is clearly wrong, given that we do compile with -fopenmp and would normally expect openmp parts to be honored.
Issuing a warning does not feel right. Producing output with a missing key functionality is a bit too severe for a warning, IMO.
Making it an error would make most sense to me, but I'll defer to OpenMP folks on this.

I would emit an error. A warning only if we can ensure the code does something sensible. Right now, I doubt that is the case, similarly I doubt we actually ignore things.

yaxunl added a comment.Mar 9 2023, 8:44 PM

HIP toolchain does not pass -fopenmp-targets=amdgcn-amd-amdhsa to clang -cc1 in host compilation. It does not pass -fopenmp-is-device to clang -cc1 in device compilation. Without these options clang will not generate OpenMP offloading code for amdgpu in device and host compilation. The host code should still function correctly.

I tend to emit warning instead of error. I am concerned that emitting error may cause regressions in existing HIP applications whereas warnings can be disabled.

mhalk added inline comments.Mar 10 2023, 9:12 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

I just re-checked the LLVM IR of an example.
With -x c++ there are __kmpc_target_(de)init calls; with -x hip there are none.
https://godbolt.org/z/fhds46Pvc

From my point of view, "effectively", target directives seem to have been ignored.
Maybe this is a coincidence (there's definitely some uncertainty) or I am misinterpreting the IR.
I would be glad if you could shed some light on this, if possible / you find the time to do so.

jdoerfert added a comment.EditedMar 10 2023, 2:25 PM
This comment has been deleted.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

Line 512 in the HIP output: https://godbolt.org/z/n5s5jz7j9.

yaxunl added inline comments.Mar 10 2023, 2:56 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

If I remove --offload-arch for the C++ program, I got the same result as for HIP:

https://godbolt.org/z/4TMzxdfj6

Does that mean we should emit the same error for C++ too?

jplehr added a subscriber: jplehr.Mar 13 2023, 6:04 AM
jdoerfert added inline comments.Mar 14 2023, 2:45 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

I honestly do not follow your logic.

As I said in the beginning: I doubt the directive is ignored.

My link showed you that it doesn't ignore the pragma, e.g., the code w/ and w/o the pragma have different results.
This is also true for C++, but that doesn't make the warning any more accurate.

yaxunl added inline comments.Mar 14 2023, 6:06 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

I understand that the directive is not ignored and incorrect IR are emitted.

What I mean is that compiling "omp target" with HIP is equivalent to compiling C++ containing "omp target" without -fopenmp-targets specified in clang FE, therefore the error should be emitted in a more generic case. That is:

if "omp target" is compiled without -fopenmp-targets specified in FE, an error should be emitted.

mhalk added a subscriber: ronlieb.Mar 15 2023, 5:59 AM
jdoerfert added inline comments.Mar 15 2023, 7:11 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8634

incorrect IR are emitted.

No, that IR is not incorrect. It is exactly what should be emitted.

if "omp target" is compiled without -fopenmp-targets specified in FE, an error should be emitted.

No it should not. OpenMP target w/o an offloading target is well defined OpenMP.

mhalk added a comment.Mar 22 2023, 5:11 AM

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

If your concern was on the clang / SemA side, I found appropriate helper functions to reduce the number of checks (if (getLangOpts().HIP) { Diag(...) }) from 19 to 4.
But e.g. in the test we will still emit 24 messages, for affected directives.

I doubt reducing the number of diagnostic messages is worth the effort and esp. cost during compilation when tackled e.g. via clang-tooling?! (unsure)
The idea being: emission of a single diagnostic message and pointing out all (24) affected locations.

I may be completely wrong, but I suspect that users affected by "using HIP language but OpenMP target offloading behavior is altered" will (want to) disable this particular diagnostic once they are "aware".

clang/include/clang/Basic/DiagnosticSemaKinds.td
8635

I would emit an error. A warning only if we can ensure the code does something sensible. Right now, I doubt that is the case, similarly I doubt we actually ignore things.

@yaxunl Just wanted to point out: If we added DefaultError here, the SemA would emit an error as preferred by Johannes.
But one could still disable it via -Wno-hip-omp-target-directives even with -Werror (or also demote it via -Wno-error=hip-omp-target-directives).

Would that be acceptable for everyone?

yaxunl added inline comments.Mar 22 2023, 7:46 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8635

LGTM

mhalk updated this revision to Diff 507409.Mar 22 2023, 9:53 AM

Patch rework, implementing the mentioned changes.

jdoerfert resigned from this revision.Mar 22 2023, 4:58 PM
jdoerfert added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8635

That sounds sensible.

yaxunl accepted this revision.Mar 28 2023, 8:51 AM

LGTM. Thanks

This revision is now accepted and ready to land.Mar 28 2023, 8:51 AM