[flang][OpenMP] Parallel region codegen support #424

SouraVX · 2020-09-13T19:12:30Z

Executable can be generated and tested as:

$ bbc -fopenmp -emit-fir parallel.f90 -o -| tco | llc -filetype=obj -o parallel.o
$ clang parallel.o -L/PATH/lib -lFortranRuntime -lFortranDecimal -L/PATH/lib/ -lomp -lstdc++ -lm
$ ./a.out

SouraVX · 2020-09-13T19:14:04Z

Pattern matching at LLVMIR level is getting a bit problematic to deal with.

kiranchandramohan

Thanks @SouraVX. Great going!
A few comments inline.

kiranchandramohan · 2020-09-14T16:54:03Z

flang/include/flang/Lower/OpenMP.h

 void genOpenMPConstruct(AbstractConverter &, pft::Evaluation &,
-                        const parser::OpenMPConstruct &);
+                        const parser::OpenMPConstruct &, GenFIRCBTy genFIRCB);


Are callbacks used to match the OpenMPIRBuilder style? Or do you think it is necessary here?

Using Callbacks approach has following advantages:

No duplication, otherwise might need to pull out lot of code from Bridge.cpp(where most of the lowering code is residing).

Less error prone, since for actual codegen we fall back to Flang codegen cleanly.

Code flow and interfacing of OpenMP codegen from rest of the codegen remains pretty independent/good/.

kiranchandramohan · 2020-09-14T16:58:48Z

flang/lib/Lower/OpenMP.cpp

@@ -21,6 +21,8 @@

 #define TODO() llvm_unreachable("not yet implemented")

+using GenFIRCBTy = std::function<void(Fortran::lower::pft::Evaluation &eval)>;


Is this a re-declaration? There is one in OpenMP.h as well.

Yes, that's needed. I cross-checked once again :)

kiranchandramohan · 2020-09-14T17:00:52Z

flang/test/Lower/OpenMP/omp-parallel-region.f90

@@ -0,0 +1,84 @@
+! This test checks lowering of OpenMP parallel Directive with arbitrary code


Can you add more tests?

With a fir loop inside.

With an if statement.

with a goto statement (branching to inside the region).

Sure will take care of this in next revision.

SouraVX · 2020-09-16T08:37:07Z

do loop test: is failing at legalization to LLVMIR Dialect. I'm looking into it.

program main
!$OMP PARALLEL NUM_THREADS(2)
      do i = 2, 5
      print*, "Hello"
      end do
!$OMP END PARALLEL
end

bbc -fopenmp loop.f90 -emit-fir -o - |tco

loc("<stdin>":24:13): error: failed to legalize operation 'std.addi'
error: error in converting to LLVM-IR dialect

kiranchandramohan · 2020-09-16T08:47:36Z

Might need the conversion pattern check openmptollvm in the conversions directory.

SouraVX · 2020-09-16T10:14:07Z

Thanks @kiranchandramohan, legalization worked. But some invalid type conversion getting triggered --- related to fir.
@schweitzpgi Any hints ? is this sort of bug reported previously ?

bbc -fopenmp loop.f90 --dump-module-on-failure -emit-llvm
error: 'fir.convert' op invalid type conversion
oops, pass manager reported failure
....
%0 = llvm.mlir.constant(2 : i32) : !llvm.i32
      %1 = llvm.mlir.constant(2 : index) : !llvm.i64
      %2 = llvm.mlir.constant(1 : index) : !llvm.i64
      %3 = llvm.mlir.constant(-1 : i32) : !llvm.i32
      %4 = llvm.mlir.constant(4 : i32) : !llvm.i32
      %5 = llvm.mlir.constant(5 : index) : !llvm.i64
      %6 = llvm.mlir.constant(4 : index) : !llvm.i64
      %7 = llvm.mlir.constant(0 : index) : !llvm.i64

....
omp.parallel num_threads(%0 : !llvm.i32) {
      llvm.br ^bb1(%1, %1, %6 : !llvm.i64, !llvm.i64, !llvm.i64)
    ^bb1(%9: !llvm.i64, %10: !llvm.i64, %11: !llvm.i64):  // 2 preds: ^bb0, ^bb2
      %12 = llvm.icmp "sgt" %11, %7 : !llvm.i64
      llvm.cond_br %12, ^bb2, ^bb3
    ^bb2:  // pred: ^bb1
      %13 = fir.convert %9 : (!llvm.i64) -> i32  // This is triggering invalid type conversion
      fir.store %13 to %8 : !fir.ref<i32>
      %14 = fir.address_of(@_QQcl.6C6F6F702E663930) : !fir.ref<!fir.array<9x!fir.char<1>>>
      %15 = fir.convert %14 : (!fir.ref<!fir.array<9x!fir.char<1>>>) -> !fir.ref<i8>
      %16 = fir.call @_FortranAioBeginExternalListOutput(%3, %15, %4) : (!llvm.i32, !fir.ref<i8>, !llvm.i32) -> !fir.ref<i8>
      %17 = fir.address_of(@_QQcl.48656C6C6F) : !fir.ref<!fir.array<5x!fir.char<1>>>
      %18 = fir.convert %17 : (!fir.ref<!fir.array<5x!fir.char<1>>>) -> !fir.ref<i8>
      %19 = fir.convert %5 : (!llvm.i64) -> i64
      %20 = fir.call @_FortranAioOutputAscii(%16, %18, %19) : (!fir.ref<i8>, !fir.ref<i8>, i64) -> i1
      %21 = fir.call @_FortranAioEndIoStatement(%16) : (!fir.ref<i8>) -> i32
      %22 = llvm.add %9, %2 : !llvm.i64
      %23 = llvm.sub %11, %2 : !llvm.i64
      llvm.br ^bb1(%22, %22, %23 : !llvm.i64, !llvm.i64, !llvm.i64)
    ^bb3:  // pred: ^bb1
      %24 = fir.convert %10 : (!llvm.i64) -> i32
      fir.store %24 to %8 : !fir.ref<i32>
      omp.terminator
...
    }

kiranchandramohan · 2020-09-16T10:35:46Z

There is probably an ordering issue here. The openmp to llvm conversion is not aware of fir. I was hopping that the fir to llvm pass will be called automatically. Will swapping the order of fir to llvm and openmp to llvm get it to work? @ericschweitz can probably help here.

SouraVX · 2020-09-16T10:45:23Z

There is probably an ordering issue here. The openmp to llvm conversion is not aware of fir. I was hopping that the fir to llvm pass will be called automatically. Will swapping the order of fir to llvm and openmp to llvm get it to work? @ericschweitz can probably help here.

Yes, observing same. IIUC due to current ordering this pass is lowering all std operations to llvm dialect too early and once the operation is lowered it's probably not feasible for FIR to lift up the abstraction and work.
After inserting this pass in pipeline noting following failures:

Failed Tests (5):
  Flang :: Lower/OpenMP/empty-omp-parallel.f90
  Flang :: Lower/OpenMP/omp-parallel-region.f90
  Flang :: Lower/array-init.f90
  Flang :: Lower/end-to-end-character-assignment.f90
  Flang :: Lower/intrinsic-wrappers.f90

Will swapping the order of fir to llvm and openmp to llvm get it to work?

Swapping or ordering this way works fine: but earlier issue persist(i.e failed to legalize operation 'std.addi')

 pm.addPass(fir::createFIRToLLVMPass(nameUniquer));
 pm.addPass(mlir::createConvertOpenMPToLLVMPass());

kiranchandramohan · 2020-09-16T10:54:14Z

May be what is required is an openmp to fir pass.

kiranchandramohan · 2020-09-16T12:14:42Z

Will adding the parallel conversion pattern in the fir to llvm pass make it work? You will have to mark the parallel operation as illegal.

SouraVX · 2020-09-17T18:56:03Z

@kiranchandramohan, this error:

loc("<stdin>":24:13): error: failed to legalize operation 'std.addi'
error: error in converting to LLVM-IR dialect

is due to Standard Dialect is not registered as Legal Dialect in: Adding it as legal doesn't make sense, since we added conversion patterns populated there it self.

f18-llvm-project/flang/lib/Optimizer/CodeGen/CodeGen.cpp

Line 2778 in 4b964eb

target.addLegalDialect<mlir::omp::OpenMPDialect>();

After investigating through lowering part, I'm also sceptic WRT lowering that has happened. Code inside the region utilizes fir lowering code.
Then how come it's creating Operation from Standard Dialect it should've created fir analog of these operations ?

 %14 = addi %1, %c1 : index
 %15 = subi %3, %c1 : index

@schweitzpgi @jeanPerier , do you guys think this lowering is legit ?

Also I had a brief discussion with @kiranchandramohan , he was suggesting to proceed with this PR without the Loop test case and to handle this issue separately.
Does that sounds good to you folks ?

schweitzpgi · 2020-09-17T23:02:54Z

This looks strange and (likely) is failing legalization.

%19 = fir.convert %5 : (!llvm.i64) -> i64

Are you converting OMP dialect to standard dialect or straight to LLVM-IR? Code gen to LLVM IR dialect in tco does both standard and fir dialects together. It should be easy enough to add more patterns.

clementval · 2020-09-21T01:23:43Z

flang/lib/Lower/Bridge.cpp

+    auto genFIRCB = [&](Fortran::lower::pft::Evaluation &eval) {
+      genFIR(eval, /*unstructuredContext=*/false);
+    };
+    genOpenMPConstruct(*this, getEval(), omp, genFIRCB);


You can also set the insertionPoint correctly in genOpenMPConstruct and call genFIR in the eval loop right here so you don't need to pass a callback function.

Thanks for your suggestion! it simplified the change a lot. 👍

Executable can be generated and tested as: ``` $ bbc -fopenmp -emit-fir parallel.f90 -o -| tco | llc -filetype=obj -o parallel.o $ clang parallel.o -L/PATH/lib -lFortranRuntime -lFortranDecimal -L/PATH/lib/ -lomp -lstdc++ -lm $ ./a.out ``` Extended test case for `if statement`. TODO: extend for `do loop` Added OpenMPToLLVMPass to bbc pass pipeline

SouraVX · 2020-09-21T07:22:14Z

This looks strange and (likely) is failing legalization.
%19 = fir.convert %5 : (!llvm.i64) -> i64
Are you converting OMP dialect to standard dialect or straight to LLVM-IR? Code gen to LLVM IR dialect in tco does both standard and fir dialects together. It should be easy enough to add more patterns.

This only happens if we add pm.addPass(mlir::createConvertOpenMPToLLVMPass()); pass before FIRToLLVM and we figured out that this is too early and wrong.

But primary problem still persist, we(@kiranchandramohan too) are considering to create separate issue and work on it.

loc("<stdin>":24:13): error: failed to legalize operation 'std.addi'
error: error in converting to LLVM-IR dialect

I've squashed this PR.
Does that sound good to you @schweitzpgi ?

kiranchandramohan · 2020-09-21T22:01:14Z

Yes, I agree with @SouraVX that we can handle the ordering of lowering passes or addition of conversion patterns as a separate patch. If the basic lowering of a parallel region works in this patch then that is fine.

kiranchandramohan

LGTM.

schweitzpgi · 2020-09-22T15:25:11Z

I'm good with making progress and noting TODO items.

I assume this is ready to merge now?

After skeleton of the `Parallel Op` is created set the insertion point to start of the block. So that later `CodeGen` can proceed. Note: This patch reflects the work that can be upstreamed from PR(merged) PR: flang-compiler#424 Reviewed By: schweitz, kiranchandramohan Differential Revision: https://reviews.llvm.org/D88221

After skeleton of the `Parallel Op` is created set the insertion point to start of the block. So that later `CodeGen` can proceed. Note: This patch reflects the work that can be upstreamed from PR(merged) PR: flang-compiler/f18-llvm-project#424 Reviewed By: schweitz, kiranchandramohan Differential Revision: https://reviews.llvm.org/D88221

schweitzpgi requested review from vdonaldson and kiranchandramohan September 14, 2020 15:30

kiranchandramohan reviewed Sep 14, 2020

View reviewed changes

schweitzpgi force-pushed the fir-dev branch from 4c3904b to 9cdf257 Compare September 15, 2020 15:48

SouraVX force-pushed the parallel-codegen branch from 2607340 to edfb6c9 Compare September 16, 2020 08:33

clementval reviewed Sep 21, 2020

View reviewed changes

SouraVX force-pushed the parallel-codegen branch from ebe0d09 to 4f1e425 Compare September 21, 2020 07:16

SouraVX requested review from clementval and kiranchandramohan September 21, 2020 07:22

kiranchandramohan approved these changes Sep 21, 2020

View reviewed changes

schweitzpgi approved these changes Sep 22, 2020

View reviewed changes

schweitzpgi requested a review from jeanPerier September 22, 2020 15:26

schweitzpgi merged commit 2f3a180 into flang-compiler:fir-dev Sep 22, 2020

sitio-couto mentioned this pull request Jul 29, 2021

How to compile FIR to an executable binary? #948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][OpenMP] Parallel region codegen support #424

[flang][OpenMP] Parallel region codegen support #424

SouraVX commented Sep 13, 2020

SouraVX commented Sep 13, 2020 •

edited

kiranchandramohan left a comment

kiranchandramohan Sep 14, 2020

SouraVX Sep 15, 2020

kiranchandramohan Sep 14, 2020

SouraVX Sep 15, 2020

kiranchandramohan Sep 14, 2020

SouraVX Sep 15, 2020

SouraVX commented Sep 16, 2020

kiranchandramohan commented Sep 16, 2020

SouraVX commented Sep 16, 2020 •

edited

kiranchandramohan commented Sep 16, 2020

SouraVX commented Sep 16, 2020

kiranchandramohan commented Sep 16, 2020

kiranchandramohan commented Sep 16, 2020

SouraVX commented Sep 17, 2020

schweitzpgi commented Sep 17, 2020

clementval Sep 21, 2020 •

edited

SouraVX Sep 21, 2020

SouraVX commented Sep 21, 2020 •

edited

kiranchandramohan commented Sep 21, 2020

kiranchandramohan left a comment

schweitzpgi commented Sep 22, 2020

		@@ -21,6 +21,8 @@

		#define TODO() llvm_unreachable("not yet implemented")

		using GenFIRCBTy = std::function<void(Fortran::lower::pft::Evaluation &eval)>;

		@@ -0,0 +1,84 @@
		! This test checks lowering of OpenMP parallel Directive with arbitrary code

[flang][OpenMP] Parallel region codegen support #424

[flang][OpenMP] Parallel region codegen support #424

Conversation

SouraVX commented Sep 13, 2020

SouraVX commented Sep 13, 2020 • edited

kiranchandramohan left a comment

Choose a reason for hiding this comment

kiranchandramohan Sep 14, 2020

Choose a reason for hiding this comment

SouraVX Sep 15, 2020

Choose a reason for hiding this comment

kiranchandramohan Sep 14, 2020

Choose a reason for hiding this comment

SouraVX Sep 15, 2020

Choose a reason for hiding this comment

kiranchandramohan Sep 14, 2020

Choose a reason for hiding this comment

SouraVX Sep 15, 2020

Choose a reason for hiding this comment

SouraVX commented Sep 16, 2020

kiranchandramohan commented Sep 16, 2020

SouraVX commented Sep 16, 2020 • edited

kiranchandramohan commented Sep 16, 2020

SouraVX commented Sep 16, 2020

kiranchandramohan commented Sep 16, 2020

kiranchandramohan commented Sep 16, 2020

SouraVX commented Sep 17, 2020

schweitzpgi commented Sep 17, 2020

clementval Sep 21, 2020 • edited

Choose a reason for hiding this comment

SouraVX Sep 21, 2020

Choose a reason for hiding this comment

SouraVX commented Sep 21, 2020 • edited

kiranchandramohan commented Sep 21, 2020

kiranchandramohan left a comment

Choose a reason for hiding this comment

schweitzpgi commented Sep 22, 2020

SouraVX commented Sep 13, 2020 •

edited

SouraVX commented Sep 16, 2020 •

edited

clementval Sep 21, 2020 •

edited

SouraVX commented Sep 21, 2020 •

edited