[PATCH] D147941: [Flang][Driver][OpenMP] Enable options for selecting offloading phases in flang

[Sergio Afonso via Phabricator via cfe-commits]

skatrak marked 4 inline comments as done.
skatrak added a comment.

In D147941#4255622 <https://reviews.llvm.org/D147941#4255622>, @awarzynski wrote:

> In the context of LLVM, I would normally associate "pass" with something else. I'm not that familiar with offloading, so perhaps that's the language that people use? Personally, in the context of the driver I'd normally use the term "phase" rather than "pass".
>
> This patch makes sense, though I'd like the following to be addressed before landing this:
>
> 1. Where's the logic that implements what `--offload-host-device`? It looks like this is already implemented elsewhere and this patch simply "unlocks" (rather than "implements") this option for Flang.
> 2. The name of "omp-frontend-forwarding.f90" is very misleading. "Forwarding" would mean `flang-new --offload-host-only %s` --> `flang-new -fc1 --offload-host-only %s`, but that's not what's happening here, is it?
>
> For 1. you could just update the summary, and for 2. I suggest renaming "omp-frontend-forwarding.f90" as e.g. "omp-compiler-flag-expansion.f90". WDYT?

I updated the patch title and summary, and renamed the test file to something that I think represents better the contents of the test. Thank you again for the suggestions, let me know if there are any other improvements to make before landing this patch.



================
Comment at: clang/include/clang/Driver/Options.td:2738
   HelpText<"Don't Use the new driver for offloading compilation.">;
-def offload_device_only : Flag<["--"], "offload-device-only">,
+def offload_device_only : Flag<["--"], "offload-device-only">, Flags<[CoreOption, FlangOption]>,
   HelpText<"Only compile for the offloading device.">;
----------------
awarzynski wrote:
> Why is `CoreOption` needed here? Wouldn't `FlangOption` be sufficient?
You're right, thanks for the comment.


================
Comment at: flang/test/Driver/omp-frontend-forwarding.f90:30
+
+! CHECK-OFFLOAD-HOST: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu"
+! CHECK-OFFLOAD-HOST-NOT: "-triple" "amdgcn-amd-amdhsa"
----------------
awarzynski wrote:
> Shouldn't there be 2 driver invocation for the host, as in the the `CHECK-OFFLOAD-HOST-DEVICE` case?
In this case only the first invocation would remain, but I added an additional `OFFLOAD-HOST-NOT` to make this behavior explicit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147941