[flang] do not merge block after lowering

[jeanPerier]

Summary

Lowering relies on dead code generation / unreachable block deletion
to delete some code that is potentially invalid.

However, calling mlir::simplifyRegion also merges block, which may
promote SSA values to block arguments. Not all FIR types are intended
to be block arguments.
The added test shows an example where block merging led to
fir.shape<> being block arguments (and a failure later in codegen).

[clementval]

LGTM

[vdonaldson]

Does block coalescing still happen later in the compilation pipeline? That's a nice optimization that we don't want to completely lose.

flang/lib/Lower/Bridge.cpp

4181

Is this FIXME comment (still) valid?

[jeanPerier]

In D153858#4452828, @vdonaldson wrote:

Does block coalescing still happen later in the compilation pipeline? That's a nice optimization that we don't want to completely lose.

It will still happen in LLVM after MLIR depending on what optimization levels are set.

If we want it at the MLIR level, we will need to add options to the MLIR one so that it has some sort of idea of the cost of merging blocks (creating block arguments implies forcing the codegen of the related type to some runtime representation. Depending on the SSA type this may or may not be cheap. So the MLIR pass should be provided some kind of interface to decide whether or not block merging is profitable).
Or we will need to share the some logic with MLIR but do our own pass.

So far I am assuming that LLVM is able to do a good job for simple small blocks/extra jumps and that we do not really need to do this at the MLIR level.

flang/lib/Lower/Bridge.cpp

4181

It is not recommended by MLIR to use IRRewriter directly, that is why I am leaving it. But I do not see us changing this anytime soon.

[vdonaldson]

Thanks for the explanations. Looks ok to me.

[tblah]

LGTM

[kiranchandramohan]

In D153858#4452968, @jeanPerier wrote:

In D153858#4452828, @vdonaldson wrote:

Does block coalescing still happen later in the compilation pipeline? That's a nice optimization that we don't want to completely lose.

It will still happen in LLVM after MLIR depending on what optimization levels are set.

If we want it at the MLIR level, we will need to add options to the MLIR one so that it has some sort of idea of the cost of merging blocks (creating block arguments implies forcing the codegen of the related type to some runtime representation. Depending on the SSA type this may or may not be cheap. So the MLIR pass should be provided some kind of interface to decide whether or not block merging is profitable).
Or we will need to share the some logic with MLIR but do our own pass.

So far I am assuming that LLVM is able to do a good job for simple small blocks/extra jumps and that we do not really need to do this at the MLIR level.

Canonicalization can also merge blocks. Would we run into the same issue (with block arguments) there?

flang/lib/Lower/Bridge.cpp

   }
 
   /// Conditionally switch code insertion to a new block.
   void maybeStartBlock(mlir::Block *newBlock) {
     if (newBlock)
       startBlock(newBlock);
   }
 
+  void eraseDeadCodeAndBlocks(mlir::RewriterBase &rewriter,
+                              llvm::MutableArrayRef<mlir::Region> regions) {
+    (void)mlir::eraseUnreachableBlocks(rewriter, regions);
+    (void)mlir::runRegionDCE(rewriter, regions);
+  }
+
   /// Finish translation of a function.
   void endNewFunction(Fortran::lower::pft::FunctionLikeUnit &funit) {
     setCurrentPosition(Fortran::lower::pft::stmtSourceLoc(funit.endStmt));
     if (funit.isMainProgram()) {
       bridge.fctCtx().finalizeAndPop();
       genExitRoutine();
     } else {
       genFIRProcedureExit(funit, funit.getSubprogramSymbol());
     }
     funit.finalBlock = nullptr;
     LLVM_DEBUG(llvm::dbgs() << "\n[bridge - endNewFunction";
                if (auto *sym = funit.scope->symbol()) llvm::dbgs()
                << " " << sym->name();
                llvm::dbgs() << "] generated IR:\n\n"
                             << *builder->getFunction() << '\n');
     // Eliminate dead code as a prerequisite to calling other IR passes.
     // FIXME: This simplification should happen in a normal pass, not here.
     mlir::IRRewriter rewriter(*builder);
-    (void)mlir::simplifyRegions(rewriter, {builder->getRegion()});
+    (void)eraseDeadCodeAndBlocks(rewriter, {builder->getRegion()});
     delete builder;
     builder = nullptr;
     hostAssocTuple = mlir::Value{};
     localSymbols.clear();
     blockId = 0;
   }
 
   /// Helper to generate GlobalOps when the builder is not positioned in any

flang/test/Lower/HLFIR/no-block-merging.f90

+! Test that dead code elimination after lowering does not merge
+! blocks. This test was added because block merging on the
+! code below created a block with a fir.shape<> block argument
+! which FIR codegen is not intended to support.
+! RUN: bbc -emit-hlfir -o - %s | FileCheck %s
+
+subroutine test(res, selector, x1, x2, x3, x4, x5, x6, x7, x8)
+  integer :: res
+  character(*) :: selector
+  real(8), dimension(7) :: x1, x2, x3, x4, x5, x6, x7, x8
+  interface
+   integer function foo(x)
+     real(8) :: x(:)
+   end function
+  end interface
+  select case (selector)
+    case ("01")
+      res = foo(x1)
+      res = foo(x1)
+    case ("02")
+      res = foo(x2)
+      res = foo(x2)
+    case ("03")
+      res = foo(x3)
+      res = foo(x3)
+    case ("04")
+      res = foo(x4)
+      res = foo(x4)
+    case ("05")
+      res = foo(x5)
+      res = foo(x5)
+    case ("06")
+      res = foo(x6)
+      res = foo(x6)
+    case ("07")
+      res = foo(x7)
+      res = foo(x7)
+    case ("08")
+      res = foo(x8)
+      res = foo(x8)
+    case default
+   end select
+end subroutine
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo
+! CHECK: fir.call @_QPfoo

flang/test/Lower/block.f90

 ! RUN: bbc -emit-fir -o - %s | FileCheck %s
 
 ! CHECK-LABEL: func @_QQmain
 program bb ! block stack management and exits
     ! CHECK:   %[[V_1:[0-9]+]] = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFEi"}
     integer :: i, j
     ! CHECK:   fir.store %c0{{.*}} to %[[V_1]] : !fir.ref<i32>
     i = 0
     ! CHECK:   %[[V_3:[0-9]+]] = fir.call @llvm.stacksave()
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
     ! CHECK:   br ^bb1
-    ! CHECK: ^bb1:  // 2 preds: ^bb0, ^bb15
-    ! CHECK:   cond_br %{{.*}}, ^bb2, ^bb16
+    ! CHECK: ^bb1:  // 2 preds: ^bb0, ^bb16
+    ! CHECK:   cond_br %{{.*}}, ^bb2, ^bb17
     ! CHECK: ^bb2:  // pred: ^bb1
     ! CHECK:   %[[V_11:[0-9]+]] = fir.call @llvm.stacksave()
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
     ! CHECK:   cond_br %{{.*}}, ^bb3, ^bb4
     ! CHECK: ^bb3:  // pred: ^bb2
     ! CHECK:   br ^bb10
     ! CHECK: ^bb4:  // pred: ^bb2
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
     ! CHECK:   cond_br %{{.*}}, ^bb5, ^bb6
     ! CHECK: ^bb5:  // pred: ^bb4
-    ! CHECK:   br ^bb7
+    ! CHECK:   br ^bb14
     ! CHECK: ^bb6:  // pred: ^bb4
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
     ! CHECK:   cond_br %{{.*}}, ^bb7, ^bb8
-    ! CHECK: ^bb7:  // 3 preds: ^bb5, ^bb6, ^bb12
+    ! CHECK: ^bb7:  // pred: ^bb6
     ! CHECK:   fir.call @llvm.stackrestore(%[[V_11]])
-    ! CHECK:   br ^bb14
+    ! CHECK:   br ^bb15
     ! CHECK: ^bb8:  // pred: ^bb6
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
     ! CHECK:   cond_br %{{.*}}, ^bb9, ^bb10
     ! CHECK: ^bb9:  // pred: ^bb8
     ! CHECK:   fir.call @llvm.stackrestore(%[[V_11]])
-    ! CHECK:   br ^bb15
+    ! CHECK:   br ^bb16
     ! CHECK: ^bb10:  // 2 preds: ^bb3, ^bb8
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
     ! CHECK:   cond_br %{{.*}}, ^bb11, ^bb12
     ! CHECK: ^bb11:  // pred: ^bb10
     ! CHECK:   fir.call @llvm.stackrestore(%[[V_11]])
-    ! CHECK:   br ^bb17
+    ! CHECK:   br ^bb18
     ! CHECK: ^bb12:  // pred: ^bb10
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
-    ! CHECK:   cond_br %{{.*}}, ^bb13, ^bb7
+    ! CHECK:   cond_br %{{.*}}, ^bb13, ^bb14
     ! CHECK: ^bb13:  // pred: ^bb12
     ! CHECK:   fir.call @llvm.stackrestore(%[[V_11]])
     ! CHECK:   fir.call @llvm.stackrestore(%[[V_3]])
-    ! CHECK:   br ^bb18
-    ! CHECK: ^bb14:  // pred: ^bb7
-    ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
+    ! CHECK:   br ^bb19
+    ! CHECK: ^bb14: // 2 preds: ^bb5, ^bb12
+    ! CHECK:   fir.call @llvm.stackrestore(%[[V_11]])
     ! CHECK:   br ^bb15
-    ! CHECK: ^bb15:  // 2 preds: ^bb9, ^bb14
+    ! CHECK: ^bb15:  // 2 preds: ^bb7, ^bb14
+    ! CHECK:   br ^bb16
+    ! CHECK: ^bb16:  // 2 preds: ^bb9, ^bb15
     ! CHECK:   br ^bb1
-    ! CHECK: ^bb16:  // pred: ^bb1
+    ! CHECK: ^bb17:  // pred: ^bb1
     ! CHECK:   fir.store %{{.*}} to %[[V_1]] : !fir.ref<i32>
-    ! CHECK:   br ^bb17
-    ! CHECK: ^bb17:  // 2 preds: ^bb11, ^bb16
+    ! CHECK:   cf.br ^bb18
+    ! CHECK: ^bb18:  // 2 preds: ^bb11, ^bb17
     ! CHECK:   fir.call @llvm.stackrestore(%[[V_3]])
-    ! CHECK:   br ^bb18
-    ! CHECK: ^bb18:  // 2 preds: ^bb13, ^bb17
+    ! CHECK:   br ^bb19
+    ! CHECK: ^bb19:  // 2 preds: ^bb13, ^bb18
     ! CHECK:   return
     block
       i = i + 1 ! 1 increment
       do j = 1, 5
         block
           i = i + 1; if (j == 1) goto 1   ! inner block - 5 increments, 1 goto
           i = i + 1; if (j == 2) goto 2   ! inner block - 4 increments, 1 goto
           i = i + 1; if (j == 3) goto 10  ! outer block - 3 increments, 1 goto

flang/test/Lower/entry-statement.f90

function f1(n1) result(res1)
   ! CHECK:   fir.do_loop %arg3 = %[[V_9]] to %[[V_14]] step %c1{{.*}} {
   ! CHECK:     %[[V_32:[0-9]+]] = fir.convert %[[V_0]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.array<?x!fir.char<1>>>
   ! CHECK:     %[[V_33:[0-9]+]] = fir.coordinate_of %[[V_32]], %arg3 : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
   ! CHECK:     fir.store %[[V_16]] to %[[V_33]] : !fir.ref<!fir.char<1>>
   ! CHECK:   }
   ! CHECK:   %[[V_17:[0-9]+]] = fir.load %arg2 : !fir.ref<i32>
   ! CHECK:   %[[V_18:[0-9]+]] = arith.cmpi eq, %[[V_17]], %c1{{.*}}_i32_4 : i32
   ! CHECK:   cond_br %[[V_18]], ^bb2, ^bb3
-  ! CHECK: ^bb2:  // 2 preds: ^bb1, ^bb3
-  ! CHECK:   br ^bb5
+  ! CHECK: ^bb2:  // pred: ^bb1
+  ! CHECK:   br ^bb6
   ! CHECK: ^bb3:  // pred: ^bb1
   ! CHECK:   fir.call @_QFf1Ps2(%[[V_2]]) {{.*}}: (!fir.ref<tuple<!fir.boxchar<1>, !fir.boxchar<1>>>) -> ()
   ! CHECK:   %[[V_19:[0-9]+]] = fir.load %[[V_1]] : !fir.ref<i32>
   ! CHECK:   %[[V_20:[0-9]+]] = arith.cmpi eq, %[[V_19]], %c2{{.*}}_i32 : i32
-  ! CHECK:   cond_br %[[V_20]], ^bb2, ^bb4
+  ! CHECK:   cond_br %[[V_20]], ^bb4, ^bb5
   ! CHECK: ^bb4:  // pred: ^bb3
+  ! CHECK:   cf.br ^bb6
+  ! CHECK: ^bb5:  // pred: ^bb3
   ! CHECK:   %[[V_21:[0-9]+]] = fir.address_of(@_QQcl.4320432043) : !fir.ref<!fir.char<1,5>>
   ! CHECK:   %[[V_22:[0-9]+]] = arith.cmpi slt, %c5{{.*}}, %c5{{.*}} : index
   ! CHECK:   %[[V_23:[0-9]+]] = arith.select %[[V_22]], %c5{{.*}}, %c5{{.*}} : index
   ! CHECK:   %[[V_24:[0-9]+]] = fir.convert %[[V_23]] : (index) -> i64
   ! CHECK:   %[[V_25:[0-9]+]] = arith.muli %c1{{.*}}_i64_6, %[[V_24]] : i64
   ! CHECK:   %[[V_26:[0-9]+]] = fir.convert %[[V_0]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
   ! CHECK:   %[[V_27:[0-9]+]] = fir.convert %[[V_21]] : (!fir.ref<!fir.char<1,5>>) -> !fir.ref<i8>
   ! CHECK:   fir.call @llvm.memmove.p0.p0.i64(%[[V_26]], %[[V_27]], %[[V_25]], %false{{.*}}) {{.*}}: (!fir.ref<i8>, !fir.ref<i8>, i64, i1) -> ()
   ! CHECK:   %[[V_28:[0-9]+]] = arith.subi %c5{{.*}}, %c1{{.*}} : index
   ! CHECK:   %[[V_29:[0-9]+]] = fir.undefined !fir.char<1>
   ! CHECK:   %[[V_30:[0-9]+]] = fir.insert_value %[[V_29]], %c32{{.*}}_i8_9, [0 : index] : (!fir.char<1>, i8) -> !fir.char<1>
   ! CHECK:   fir.do_loop %arg3 = %[[V_23]] to %[[V_28]] step %c1{{.*}} {
   ! CHECK:     %[[V_32]] = fir.convert %[[V_0]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.array<?x!fir.char<1>>>
   ! CHECK:     %[[V_33]] = fir.coordinate_of %[[V_32]], %arg3 : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
   ! CHECK:     fir.store %[[V_30]] to %[[V_33]] : !fir.ref<!fir.char<1>>
   ! CHECK:   }
   ! CHECK:   fir.call @_QFf1Ps3(%[[V_2]]) {{.*}}: (!fir.ref<tuple<!fir.boxchar<1>, !fir.boxchar<1>>>) -> ()
-  ! CHECK:   br ^bb5
-  ! CHECK: ^bb5:  // 2 preds: ^bb2, ^bb4
+  ! CHECK:   br ^bb6
+  ! CHECK: ^bb6:  //  3 preds: ^bb2, ^bb4, ^bb5
   ! CHECK:   %[[V_31:[0-9]+]] = fir.emboxchar %[[V_0]], %c5{{.*}} : (!fir.ref<!fir.char<1,?>>, index) -> !fir.boxchar<1>
   ! CHECK:   return %[[V_31]] : !fir.boxchar<1>
   ! CHECK: }
   character(5) res1, f2, f3
   res1 = 'a a a'
   if (n1 == 1) return
 
 ! CHECK-LABEL: @_QPf2