[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+    for (size_t i = 1; i < Inputs.size(); ++i) {
+      if (Inputs[i].getType() != types::TY_Nothing)
----------------
agozillon wrote:
> awarzynski wrote:
> > What's the magic "1"? And given that the input count matters here - is there a test with multiple inputs?
> It aims to mimic the behavior of Clang: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561 where the main input is skipped (the input currently being compiled or embedded into etc.), when adding to //-fembed-offload-object//. 
> 
> It does look different to Clang's as Clang has more cases and the logic is spread across the constructJob invocation, but the first if case is what the if statement inside of the loop and setting the loop index variable to 1 do. The HostOffloadingInputs array is what is being generated here, except we're skipping and directly applying it as arguments.
> 
> I tried to condense it a little in this case! Perhaps it loses readability though, I had hoped the comment might have kept it clear
Thanks for the link - that code in Clang doesn't really clarify what makes `Inputs[0]` special 🤔 . 

Let me rephrase my question - what's so special about the first input? (referred to in Clang as "main input") Is that something specific to OpenMP? For example, in this case:
```
flang-new  -fopenmp  file.f90
```
I assume that `inputs[0]` is "file.f90", so nothing will happen?

> I tried to condense it a little in this case! Perhaps it loses readability though, I had hoped the comment might have kept it clear

Nah, I think that your implementation is fine. It's my ignorance with respect to OpenMP that's the problem here ;-)


================
Comment at: clang/test/Driver/flang/flang-omp.f90:6
+! Test regular -fopenmp with no offload
+! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck --check-prefixes=CHECK-OPENMP %s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"
----------------
agozillon wrote:
> awarzynski wrote:
> > Can you remind me why isn't it possible to test this with `flang-new`? 
> I double checked, it seems possible to test these with flang-new directly, the main reason I've tested it like this is as it's based on the other tests in the same directory which use clang! 
> 
> However, I'm more than happy to move these tests to the omp-frontend-forwarding.f90 test, remove them or keep these and add flang-new variations into omp-frontend-forwarding.f90. 
I know that it's a bit confusing - Flang.cpp lives in Clang rather than Flang. But that's because `Flang.cpp` is part of `clangDriver` - the driver library. That library is shared between Clang and Flang and in principle should be taken out of Clang into a dedicated subproject - that's the plan :)

This is effectively a Flang patch and I would prefer this test to be added in Flang (with `clang` being replaced with `flang-new`). In general, I wouldn't add any test in Clang unless testing something Clang specific. This test, however, tests Flang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815