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

Mon Mar 6 12:12:27 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:
> > > > 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! 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864