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

Wed Mar 22 05:11:15 PDT 2023

mhalk added a comment.

In D145591#4182360 <https://reviews.llvm.org/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 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".

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8635
+  "HIP does not support OpenMP target directives; directive has been ignored">,
+  InGroup<HIPOpenMPOffloading>;
+
----------------
>>! In D145591#4182945, @jdoerfert wrote:
> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145591