awarzynski added a comment. In D147941#4255461 <https://reviews.llvm.org/D147941#4255461>, @skatrak wrote:
> In D147941#4255458 <https://reviews.llvm.org/D147941#4255458>, @awarzynski > wrote: > >> Could you add tests that demonstrate what these options actually do? > > Thank you for the quick review! These options just modify which `flang-new > -fc1` invocations are produced by the driver when compiling for device > offloading. I have added tests that check that only the expected invocations > are present, but if these tests are not what you'd expect I'd gladly make > some more if you can explain a bit further what you had in mind. Cheers! > These can be used to modify the behavior of the driver to select which > compilation passes are triggered during OpenMP offloading. 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? ================ 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.">; ---------------- Why is `CoreOption` needed here? Wouldn't `FlangOption` be sufficient? ================ Comment at: flang/test/Driver/omp-frontend-forwarding.f90:14 +! RUN: --target=aarch64-unknown-linux-gnu \ +! RUN: | FileCheck %s --check-prefix=CHECK-OFFLOAD-HOST-DEVICE + ---------------- [nit] You can reduce noise by using `OFFLOAD-HOST-DEVICE` instead of `CHECK-OFFLOAD-HOST-DEVICE`. I think that most people understand that these are `CHECK` prefixes anyway. ================ Comment at: flang/test/Driver/omp-frontend-forwarding.f90:19 +! RUN: --target=aarch64-unknown-linux-gnu \ +! RUN: | FileCheck %s --check-prefix=CHECK-OFFLOAD-HOST-DEVICE + ---------------- [nit] No harm in being a bit more verbose to communicate the intent a bit clearer :) ================ 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" ---------------- Shouldn't there be 2 driver invocation for the host, as in the the `CHECK-OFFLOAD-HOST-DEVICE` case? 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
