[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

Tue Mar 7 04:43:14 PST 2023

agozillon 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
----------------
awarzynski wrote:
> 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?
Thank you very much for the link! Very useful and comprehensive documentation :)

1) That is correct, and it works like -fopenmp-is-device in Clang
2) That is correct unfortunately, but we do fully intend to support fopenmp-targets in the very near future (another team member is working on this patch), I just thought it would be nice to have all the fopenmp-is-device components in a single patch!

I have added it to the summary, sorry for missing it previously!

It's not needed within this patch no, but it does keep it self contained within one patch and other patches can build off of it a little easier, but I am happy to separate the logic into two different patches if that is preferred! 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864