[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

This revision was automatically updated to reflect the committed changes. Closed by commit rG55916de2d377: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading (authored by Michael Halkenhaeuser michaelgerald.halkenhau...@amd.com). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl accepted this revision. yaxunl added a comment. This revision is now accepted and ready to land. LGTM. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145591/new/ https://reviews.llvm.org/D145591

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jdoerfert resigned from this revision. jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8635 + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; + yaxunl wrote: > mhalk wrote:

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

mhalk updated this revision to Diff 507409. mhalk added a comment. Patch rework, implementing the mentioned changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145591/new/ https://reviews.llvm.org/D145591 Files:

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8635 + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; + mhalk wrote: > >>! In D145591#4182945, @jdoerfert wrote: > > I would

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

mhalk added a comment. In D145591#4182360 , @jhuber6 wrote: > 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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def warn_hip_omp_target_directives : Warning< + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; yaxunl wrote: >

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def warn_hip_omp_target_directives : Warning< + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; jdoerfert wrote: > yaxunl

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def warn_hip_omp_target_directives : Warning< + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; yaxunl wrote: >

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def warn_hip_omp_target_directives : Warning< + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; jdoerfert wrote: > mhalk

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jdoerfert added a comment. 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. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

mhalk added inline comments. Herald added a subscriber: sunshaoce. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def warn_hip_omp_target_directives : Warning< + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup;

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added a comment. 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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jdoerfert added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

tra added a comment. In D145591#4182737 , @yaxunl wrote: > 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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added a comment. In D145591#4182788 , @jhuber6 wrote: > In D145591#4182748 , @yaxunl wrote: > >> In D145591#4182360 , @jhuber6 >> wrote: >> >>> I'm not a fan of

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jhuber6 added a comment. In D145591#4182748 , @yaxunl wrote: > In D145591#4182360 , @jhuber6 wrote: > >> I'm not a fan of the same warning being copied in 24 places. Why do we set >> `LangOpts.IsOpenMP` on the

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added a comment. In D145591#4182360 , @jhuber6 wrote: > 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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added a comment. In D145591#4182351 , @tra wrote: > In D145591#4182168 , @yaxunl wrote: > >> -x hip and -fopenmp has been a valid combination. -fopenmp with -x hip >> allows non-offloading OpenMP

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8634 +def warn_hip_omp_target_directives : Warning< + "HIP does not support OpenMP target directives; directive has been ignored">, + InGroup; I doubt the ignored

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

jhuber6 added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

tra added a comment. In D145591#4182168 , @yaxunl wrote: > -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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

yaxunl added a comment. In D145591#4182016 , @tra wrote: > 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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

tra added a subscriber: jhuber6. tra added a comment. In D145591#4180908 , @mhalk wrote: > For example: when using `-x hip -fopenmp --offload-arch=...` (+ other > reasonable parameters) a HIP program with OpenMP target directives will > compile without

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

mhalk added a comment. In D145591#4179144 , @tra wrote: > 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

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

tra added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

scchan added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145591/new/ https://reviews.llvm.org/D145591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D145591: [clang][HIP][OpenMP] Add warning if mixed HIP / OpenMP offloading

mhalk created this revision. Herald added subscribers: guansong, yaxunl. Herald added a project: All. mhalk requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. Adds a warning, issued by the clang