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
----------------
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.


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

Reply via email to