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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
