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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
