awarzynski added inline comments.
================ Comment at: clang/test/Driver/flang/flang-omp.f90:1 +! Check that flang -fc1 is invoked when in --driver-mode=flang +! and the relevant openmp and openmp offload flags are utilised ---------------- agozillon wrote: > awarzynski wrote: > > agozillon wrote: > > > agozillon wrote: > > > > awarzynski wrote: > > > > > This test looks correct to me, but please note that: > > > > > > > > > > ``` > > > > > ! Check that flang -fc1 is invoked when in --driver-mode=flang > > > > > ``` > > > > > > > > > > yet (`%clang` instead of `%flang`) > > > > > > > > > > ``` > > > > > ! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck > > > > > --check-prefixes=CHECK-OPENMP %s > > > > > ``` > > > > > > > > > > I'm not really sure whether we should be testing Flang-specific logic > > > > > in Clang. Having said that, Flang does use `clangDriver` to implement > > > > > its driver :) > > > > > > > > > > You could consider using > > > > > https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90 > > > > > instead (or add an OpenMP specific file there). > > > > > > > > > > Not a blocker. > > > > Yes, I wasn't so sure either as it felt a little weird to test Flang > > > > components inside of Clang, but as you said it is a Clang toolchain > > > > (that this test is checking) and borrows from the clangDriver! > > > > > > > > I borrowed this test from other similar tests in the same folder that > > > > test other flang specific driver logic in a similar manner, but I am > > > > more than happy to add an additional flang specific driver test as you > > > > mention! > > > On further looking into the frontend-forwarding test, I don't know if it > > > is suitable for fopenmp-is-device as it is an fc1 option and won't be > > > forwarded from the flang-new frontend down to fc1 at the moment! > > > > > > I think this test will be more suitable when additional flags like the > > > fopenmp-targets (or similar flags) that are used in this test are added > > > to the Flang driver. As they spawn/propagate the openmp-is-device flag. > > > However, perhaps I am incorrect. > > > On further looking into the frontend-forwarding test, I don't know if it > > > is suitable for fopenmp-is-device as it is an fc1 option and won't be > > > forwarded from the flang-new frontend down to fc1 at the moment! > > > > It should be - that's what the logic in Flang.cpp does, no? And if it > > doesn't, is it a deliberate design decision? > I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the > Frontend driver, at least that's what it looks like from the ConstructJob > task that Clang invokes. > > It's a deliberate design decision that -fopenmp-is-device is an fc1 option, > it mimics the way Clang currently does it, which is a cc1 option, there is no > way to directly specify it to clang without -cc1 I think. The hope is to keep > the Flang OpenMP flags as similar to Clang's as possible for the time being I > believe. > I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the > Frontend driver. `flang-new -fc1` **is** the frontend driver ;-) So: * you've added the logic to forward `-fopenmp-is-device` that should work for both `flang-new` and `clang --driver-mode=flang`, but * you're only testing one of these. There should have tests for both. In particular for `flang-new` given that this patch adds new functionality to LLVM Flang. If that doesn't work, let's try to figure it out together. > The hope is to keep the Flang OpenMP flags as similar to Clang's as possible > for the time being I believe. +1 We should try as much as we can to keep Clang's and Flang's behavior consistent :) (which you do 👍🏻 ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144864/new/ https://reviews.llvm.org/D144864 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
