[flang] Add OpenMP flag to bbc

[clementval]

Summary

Add -fopenmp flag to the bbc tool.

This patch is part of the upstreaming effort from fir-dev branch.

[awarzynski]

I'm not sure what the long-term direction is here, but AFAIK this change could be avoided and flang-new could be used for OpenMP tests instead.

[clementval]

In D121118#3364024, @awarzynski wrote:

I'm not sure what the long-term direction is here, but AFAIK this change could be avoided and flang-new could be used for OpenMP tests instead.

As long as bbc is in tree and it is used I think it makes sense to have the flag.

[shraiysh]

In D121118#3364061, @clementval wrote:

In D121118#3364024, @awarzynski wrote:

I'm not sure what the long-term direction is here, but AFAIK this change could be avoided and flang-new could be used for OpenMP tests instead.

As long as bbc is in tree and it is used I think it makes sense to have the flag.

+1 on this. As long as we are keeping bbc in the tree, I think it makes sense to have it. (Long term - maybe not - does flang-new use bbc internally or is there some code duplication?)

[shraiysh]

flang/test/Lower/OpenMP/pre-fir-tree01.f90

2

[Suggestion] Maybe keep both for time being?

RUN: bbc -fopenmp ...
RUN: flang-new -fopenmp ...

[awarzynski]

I'm primarily concerned that we are diverging here from what was discussed extensively in https://reviews.llvm.org/D117781. In particular, this patch introduces extra redundancy that could be avoided (i.e. this functionality is already available in flang-new). I would really prefer to avoid this, but I also appreciate that the key goal right now is to reduce the delta between fir-dev and main.

If this is to be merged, than @shraiysh's suggestion makes a lot of sense to me. In general, I would like us to get into a habit of testing with all drivers that support a particular functionality. This way the redundancy is obvious and we have a mechanism to prevent the drivers getting out-of-sync.

@clementval I see that you are using -pft-test here. AFAIK, there are no other OpenMP tests using PFT and in general dumping PFT has never been requested/required in the context of flang-new. I've decided to add it (see https://reviews.llvm.org/D121198) so that we can focus on the overall design rather than the fine detail.

Long term - maybe not - does flang-new use bbc internally or is there some code duplication?

bbc and flang-new are completely separate. We've been trying to reduce the code duplication, but upstreaming is higher priority :) Also, apart from code duplication there's overlap in functionality (AFAIK, bbc could already be replaced with flang-new).

[shraiysh]

apart from code duplication there's overlap in functionality (AFAIK, bbc could already be replaced with flang-new).

I like the idea of replacing bbc with flang-new. Once this is done, I don't think we need any other tool (except maybe fir-opt). flang-new handles F90 to FIR. mlir-opt handles FIR to LLVM IR Dialect. mlir-translate handles LLVM IR Dialect to LLVM IR and the rest is handled by opt and other LLVM tools. All these could be exposed by flang-new so that a user only needs to use flang-new. (Not too sure about fir-opt but if we can make flang-new plug flang optimizations to mlir-opt and run them, I don't think we need this either). We just have to expose these tools using flang-new properly. (We need to have a definitive discussion about this on discourse someday).

We've been trying to reduce the code duplication, but upstreaming is higher priority :)

Ofcourse. Honestly, as long as the functionality is exposed, it doesn't matter to me where it is exposed from, given that we are heavily inclined towards retiring bbc and tco anyways. My intention behind supporting this patch was that until we make the decision about keeping/retiring bbc, we can keep improving it by treating it as a regular tool (without the bias that it is dying 😄).

[clementval]

In D121118#3366637, @awarzynski wrote:

Also, apart from code duplication there's overlap in functionality (AFAIK, bbc could already be replaced with flang-new).

To be fair this was not the case until your recent patch which was uploaded after this one.

[awarzynski]

In D121118#3366870, @shraiysh wrote:

I like the idea of replacing bbc with flang-new. Once this is done, I don't think we need any other tool (except maybe fir-opt). flang-new handles F90 to FIR. mlir-opt handles FIR to LLVM IR Dialect. mlir-translate handles LLVM IR Dialect to LLVM IR and the rest is handled by opt and other LLVM tools. All these could be exposed by flang-new so that a user only needs to use flang-new. (Not too sure about fir-opt but if we can make flang-new plug flang optimizations to mlir-opt and run them, I don't think we need this either). We just have to expose these tools using flang-new properly. (We need to have a definitive discussion about this on discourse someday).

IIUC, fir-opt and mlir-opt are here to stay. These tools are used for testing specific passes in Flang and MLIR, respectively. We won't be able to merge them because that would require MLIR becoming aware of MLIR passes in Flang (i.e. making MLIR depend on Flang). And compiler driver, e.g. flang-new, doesn't require such granularity. Users are more concerned about e.g. -O0 vs -O1. So, it makes sense for flang-new and fir-opt to remain separate.

In D121118#3366884, @clementval wrote:

In D121118#3366637, @awarzynski wrote:

Also, apart from code duplication there's overlap in functionality (AFAIK, bbc could already be replaced with flang-new).

To be fair this was not the case until your recent patch which was uploaded after this one.

Both flang-new and bbc consume Fortran source files and generate MLIR files. That's the functionality that I had in mind. IMO, https://reviews.llvm.org/D121198 doesn't change much in the grand scheme of things.

[clementval]

In D121118#3367423, @awarzynski wrote:

In D121118#3366870, @shraiysh wrote:

To be fair this was not the case until your recent patch which was uploaded after this one.

Both flang-new and bbc consume Fortran source files and generate MLIR files. That's the functionality that I had in mind. IMO, https://reviews.llvm.org/D121198 doesn't change much in the grand scheme of things.

That’s not what is tested here.

[clementval]

Maybe a solution would be to push these options in the CLOptions.inc so they are not duplicated?

[awarzynski]

Sorry, I digressed a bit.

In D121118#3367576, @clementval wrote:

Maybe a solution would be to push these options in the CLOptions.inc so they are not duplicated?

Sadly, that wouldn't help. Options for flang-new are defined in Options.td (e.g. -fopenmp). Also, bbc uses the llvm::cl API, which registers options globally. This can lead to some annoying behavior (e.g. Error: Option 'some-option' registered more than once!).

Personally, I feel that landing https://reviews.llvm.org/D121198 and then adding an extra RUN line here (with flang-new) would put us in a much better position. The redundancy would be both documented and tested at the same time.

[clementval]

In D121118#3367668, @awarzynski wrote:

Sorry, I digressed a bit.

In D121118#3367576, @clementval wrote:

Maybe a solution would be to push these options in the CLOptions.inc so they are not duplicated?

Sadly, that wouldn't help. Options for flang-new are defined in Options.td (e.g. -fopenmp). Also, bbc uses the llvm::cl API, which registers options globally. This can lead to some annoying behavior (e.g. Error: Option 'some-option' registered more than once!).

Personally, I feel that landing https://reviews.llvm.org/D121198 and then adding an extra RUN line here (with flang-new) would put us in a much better position. The redundancy would be both documented and tested at the same time.

Right! I forgot they do not use the same mechanism. Having two run lines for now is a good solution and it makes the test ready for future moves.

[clementval]

Rebase + add flang_fc1 run line

[awarzynski]

Thanks for incorporating flang-new 👍🏻 .

flang/test/Lower/OpenMP/pre-fir-tree01.f90

+! RUN: bbc -fopenmp -pft-test -o %t %s | FileCheck %s
+! RUN: %flang_fc1 -fopenmp -fdebug-dump-pft -o %t %s | FileCheck %s
+
+! Test structure of the Pre-FIR tree with OpenMP
+
+subroutine sub1(a, b, n)
+  real :: a(:), b(:)
+  integer :: n, i
+  !$omp parallel do
+  do i = 1, n
+    b(i) = exp(a(i))
+  end do
+  !$omp end parallel do
+end subroutine
+
+! CHECK-LABEL: Subroutine sub1
+! CHECK:       <<OpenMPConstruct>>
+! CHECK:       <<DoConstruct>>
+! CHECK:       <<End OpenMPConstruct>>

flang/tools/bbc/bbc.cpp

emitFIR("emit-fir",
             llvm::cl::desc("Dump the FIR created by lowering and exit"),
             llvm::cl::init(false));
 
 static llvm::cl::opt<bool> pftDumpTest(
     "pft-test",
     llvm::cl::desc("parse the input, create a PFT, dump it, and exit"),
     llvm::cl::init(false));
 
+static llvm::cl::opt<bool> enableOpenMP("fopenmp",
+                                        llvm::cl::desc("enable openmp"),
+                                        llvm::cl::init(false));
+
 #define FLANG_EXCLUDE_CODEGEN
 #include "flang/Tools/CLOptions.inc"
 
 //===----------------------------------------------------------------------===//
 
 using ProgramName = std::string;
 
 // Print the module without the "module { ... }" wrapper.
int main(int argc, char **argv) {
   options.predefinitions.emplace_back("__flang__"s, "1"s);
   options.predefinitions.emplace_back("__flang_major__"s,
                                       std::string{FLANG_VERSION_MAJOR_STRING});
   options.predefinitions.emplace_back("__flang_minor__"s,
                                       std::string{FLANG_VERSION_MINOR_STRING});
   options.predefinitions.emplace_back(
       "__flang_patchlevel__"s, std::string{FLANG_VERSION_PATCHLEVEL_STRING});
 
+  // enable parsing of OpenMP
+  if (enableOpenMP) {
+    options.features.Enable(Fortran::common::LanguageFeature::OpenMP);
+    options.predefinitions.emplace_back("_OPENMP", "201511");
+  }
+
   Fortran::common::IntrinsicTypeDefaultKinds defaultKinds;
   Fortran::parser::AllSources allSources;
   Fortran::parser::AllCookedSources allCookedSources(allSources);
   Fortran::semantics::SemanticsContext semanticsContext{
       defaultKinds, options.features, allCookedSources};
 
   return mlir::failed(convertFortranSourceToMLIR(
       inputFilename, options, programPrefix, semanticsContext, passPipe));
 }