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: > > > 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 👍🏻 ) > Ah I didn't realise that, thank you very much for clarifying that for me! :) > I thought it was bypassing it with -fc1, and yjsy flang-new without it was > the frontend! I still have a lot to learn, so I apologies for the > misunderstanding! > > In this case do you mean test with "flang-new -fc1 -fopenmp-is-device" or > "flang-new -fopenmp-is-device", as the latter will not be accepted by > flang-new in the current patch but I do test the -fc1 variation in the > omp-is-device.f90 test which is more about testing the attribute admittedly > as opposed to the driver command. > > I don't know if testing it in the frontend-forwarding.f90 test is the right > location in this case as it doesn't test with -fc1, it seems to test options > are forwarded to -fc1 correctly, I could be incorrect though. In future > passing an argument like -fopenmp-targets=amdgcn-amd-amdhsa to flang-new > would likely expand into -fopenmp-is-device (and other arguments), like it > does for Clang at the moment, so this is likely the option that would be > tested inside of frontend-forwarding.f90 (and the subsequent check-case would > make sure it expanded into -fopenmp-is-device and friends correctly). > > However, I am more than happy to add a test for the forwarding behavior, I'm > just not necessarily sure what that test would look like unfortunately, so I > appologies! > I still have a lot to learn Don't we all? :) > I apologies for the misunderstanding! No worries! This could be helpful: https://github.com/llvm/llvm-project/blob/main/flang/docs/FlangDriver.md > I don't know if testing it in the frontend-forwarding.f90 I agree that probably not. In this case, a "compiler driver" flag (` -fopenmp-targets`) implies a certain "frontend driver" flag (`"-fopenmp-is-device`). That's not really forwarding. > In future passing an argument like -fopenmp-targets=amdgcn-amd-amdhsa to > flang-new Ah, that's something I've missed and that's my ignorance kicking in, sorry. Correct me if I am wrong: 1. this patch adds `-fopnemp-is-device` to Flang's frontend driver (that's the logic in CompilerInvation.cpp), 2. the logic in Flang.cpp means that despite `flang-new` not supporting `-fopenmp-targets=` just yet, it is possible to use `clang --driver-mode=flang -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa` instead of `flang-new -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa`. I've completely missed the fact that `flang-new` is missing `-fopenmp-targets` (I would appreciate that being mentioned in the summary). Do we need to have 2. at this point? Ideally, we would add `-fopenmp-targets=` to `flang-new` first and then test with: * `flang-new -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa` (this could happen in a subsequent patch, the logic for the frontend driver is still fine). While testing with `clang` is also correct, this: * `%clang --driver-mode=flang -### -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa %s` feels rather counter-intuitive and indeed very uncommon. Just from this patch alone I feel that 2. is not yet needed and you could safely: * remove the changes in Flang.cpp, and * delete flang-omp.f90). You could revisit these once `-fopenmp-targets` is ready. It's totally fine to add "frontend driver" (`flang-new -fc1`) and "compiler driver" (`flang-new`) logic separately (just make it clear in the summary). WDYT? 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
