[MLIR][SCF] Combine adjacent scf.if with same condition

[mehdi_amini]

mlir/lib/Dialect/SCF/SCF.cpp

1289

The restriction on the first if isn't intrinsic to the fusion right? We could fuse the two even if they both return results, as long as the else branch of the second does not use a result of the first if I think.

1303

[wsmoses]

Address comment

mlir/lib/Dialect/SCF/SCF.cpp

1289

I think the condition you mention isn't quite sufficient, but ensuring the second if doesn't use the result of the first if should be sufficient. Adding a note that this can be extended to support that.

[mehdi_amini]

mlir/lib/Dialect/SCF/SCF.cpp

1289

Actually neither the then or the else branch should use the result indeed.
Can we implement the general fusion directly instead of implementing this partial one?

[wsmoses]

Permit yielded values in first scf.if

[mehdi_amini]

mlir/lib/Dialect/SCF/SCF.cpp

1308–1310

You have a getUsers() on Operation which allows to directly iterates on this nested loop (see inline edit)

1313

Nit: no need for braces here.

1318

(spurious braces)

and you can likely just assign() here:

mergedTypes.assign(nextIf.getResultTypes().begin(), nextIf.getResultTypes().end());

1326

rewriter.mergeBlocks will do the splicing, but at the end.
Can you merge into the prevIf and avoid manual splicing here?

1329

Nit: .front() would avoid derefencing an iterator (may just be more readable)

1338–1340

1363

Just like the other revision: there are far too much back() (and even back().back()!) here, I'd like to see some refactoring into better accessors.

1366–1368

1385

Can you use llvm::enumerate instead of a side indexing?

[wsmoses]

Refactor API usage

[ftynse]

mlir/lib/Dialect/SCF/SCF.cpp

1317

Ultra-nit: we tend to use the /*hasElse=*/false style.

1330–1331

setInsertionPointToEnd(combinedIf.thenBlock())

1334–1335

Nit: there is llvm::append_range.

[wsmoses]

Simplify splicing

[mehdi_amini]

mlir/lib/Dialect/SCF/SCF.cpp

1342

1343

Why do you still have some "splice" above instead of rewriter.mergeBlocks()?

It seems that these splice are actually adding blocks to the region instead of merging the blocks? I'm not sure I read the code right...

[wsmoses]

mlir/lib/Dialect/SCF/SCF.cpp

1342

I don't think that's correct since the splice moves the (optional) block from the nextIf to the new if, whereas your change would move instructions (rather than blocks).

1343

The splice is to conditionally add the blocks of the else region (if the block exists).

[mehdi_amini]

mlir/lib/Dialect/SCF/SCF.cpp

1343

Ah OK I see the logic, it is actually moving and preserving the block itself. And we need this because the else region can be empty but not the then one.
It didn't seem obvious to me on first read somehow, either I'm too tired to review right now, or some high-level comments could help along the way.

mlir/lib/Dialect/SCF/SCF.cpp

for (auto tup :
           changed = true;
         }
       }
     }
     return success(changed);
   }
 };
 
+/// Merge any consecutive scf.if's with the same condition.
+///
+///    scf.if %cond {
+///       firstCodeTrue();...
+///    } else {
+///       firstCodeFalse();...
+///    }
+///    %res = scf.if %cond {
+///       secondCodeTrue();...
+///    } else {
+///       secondCodeFalse();...
+///    }
+///
+///  becomes
+///    %res = scf.if %cmp {
+///       firstCodeTrue();...
+///       secondCodeTrue();...
+///    } else {
+///       firstCodeFalse();...
+///       secondCodeFalse();...
+///    }
+struct CombineIfs : public OpRewritePattern<IfOp> {
+  using OpRewritePattern<IfOp>::OpRewritePattern;
+
+  LogicalResult matchAndRewrite(IfOp nextIf,
+                                PatternRewriter &rewriter) const override {
+    Block *parent = nextIf->getBlock();
+    if (nextIf == &parent->front())
+      return failure();
+
+    auto prevIf = dyn_cast<IfOp>(nextIf->getPrevNode());
+    if (!prevIf)
+      return failure();
+
+    // Note: could extend this to permit results from predecessor if
+    // when those results are not used in the subsequent if.
+    if (!prevIf.results().empty())
+      return failure();
+
+    if (nextIf.condition() != prevIf.condition())
+      return failure();
+
+    rewriter.updateRootInPlace(nextIf, [&]() {
+      Block &then = prevIf.thenRegion().back();
+      assert(isa<YieldOp>(&then.back()));
+      rewriter.eraseOp(&then.back());
+      rewriter.mergeBlocks(&*nextIf.thenRegion().begin(), &then);
+      nextIf.thenRegion().getBlocks().splice(
+          nextIf.thenRegion().getBlocks().begin(),
+          prevIf.thenRegion().getBlocks());
+
+      if (!prevIf.elseRegion().empty()) {
+        if (nextIf.elseRegion().empty()) {
+          nextIf.elseRegion().getBlocks().splice(
+              nextIf.elseRegion().getBlocks().begin(),
+              prevIf.elseRegion().getBlocks());
+        } else {
+          Block &elseB = prevIf.elseRegion().back();
+          assert(isa<YieldOp>(&elseB.back()));
+          rewriter.eraseOp(&elseB.back());
+          rewriter.mergeBlocks(&*nextIf.elseRegion().begin(), &elseB);
+          nextIf.elseRegion().getBlocks().splice(
+              nextIf.elseRegion().getBlocks().begin(),
+              prevIf.elseRegion().getBlocks());
+        }
+      }
+    });
+    rewriter.eraseOp(prevIf);
+    return success();
+  }
+};
+
 } // namespace
 
 void IfOp::getCanonicalizationPatterns(RewritePatternSet &results,
                                        MLIRContext *context) {
-  results
-      .add<RemoveUnusedResults, RemoveStaticCondition, ConvertTrivialIfToSelect,
-           ConditionPropagation, ReplaceIfYieldWithConditionOrValue>(context);
+  results.add<RemoveUnusedResults, RemoveStaticCondition,
+              ConvertTrivialIfToSelect, ConditionPropagation,
+              ReplaceIfYieldWithConditionOrValue, CombineIfs>(context);
 }
 
 //===----------------------------------------------------------------------===//
 // ParallelOp
 //===----------------------------------------------------------------------===//
 
 void ParallelOp::build(
     OpBuilder &builder, OperationState &result, ValueRange lowerBounds,
     ValueRange upperBounds, ValueRange steps, ValueRange initVals,
     function_ref<void(OpBuilder &, Location, ValueRange, ValueRange)>
         bodyBuilderFn) {
   result.addOperands(lowerBounds);
   result.addOperands(upperBounds);
   result.addOperands(steps);
   result.addOperands(initVals);
   result.addAttribute(
       ParallelOp::getOperandSegmentSizeAttr(),
       builder.getI32VectorAttr({static_cast<int32_t>(lowerBounds.size()),
                                 static_cast<int32_t>(upperBounds.size()),
                                 static_cast<int32_t>(steps.size()),
                                 static_cast<int32_t>(initVals.size())}));
   result.addTypes(initVals.getTypes());
 
   OpBuilder::InsertionGuard guard(builder);
   unsigned numIVs = steps.size();
   SmallVector<Type, 8> argTypes(numIVs, builder.getIndexType());
   Region *bodyRegion = result.addRegion();
   Block *bodyBlock = builder.createBlock(bodyRegion, {}, argTypes);
 
   if (bodyBuilderFn) {
     builder.setInsertionPointToStart(bodyBlock);
     bodyBuilderFn(builder, result.location,
                   bodyBlock->getArguments().take_front(numIVs),
                   bodyBlock->getArguments().drop_front(numIVs));
   }
   ParallelOp::ensureTerminator(*bodyRegion, builder, result.location);
 }
 
 void ParallelOp::build(
     OpBuilder &builder, OperationState &result, ValueRange lowerBounds,
     ValueRange upperBounds, ValueRange steps,
     function_ref<void(OpBuilder &, Location, ValueRange)> bodyBuilderFn) {
   // Only pass a non-null wrapper if bodyBuilderFn is non-null itself. Make sure
   // we don't capture a reference to a temporary by constructing the lambda at
   // function level.
   auto wrappedBuilderFn = [&bodyBuilderFn](OpBuilder &nestedBuilder,

mlir/test/Dialect/SCF/canonicalize.mlir

 // CHECK-NEXT:     %[[if:.+]] = scf.if %arg0 -> (i32) {
 // CHECK-NEXT:       %[[sv1:.+]] = "test.get_some_value"() : () -> i32
 // CHECK-NEXT:       scf.yield %[[sv1]] : i32
 // CHECK-NEXT:     } else {
 // CHECK-NEXT:       %[[sv2:.+]] = "test.get_some_value"() : () -> i32
 // CHECK-NEXT:       scf.yield %[[sv2]] : i32
 // CHECK-NEXT:     }
 // CHECK-NEXT:     return %[[if]], %arg1 : i32, i64
+
+// -----
+
+// CHECK-LABEL: @combineIfs
+func @combineIfs(%arg0 : i1, %arg2: i64) -> i32 {
+  scf.if %arg0 {
+    "test.firstCodeTrue"() : () -> ()
+    scf.yield
+  } else {
+    "test.firstCodeFalse"() : () -> ()
+    scf.yield
+  }
+  %res = scf.if %arg0 -> i32 {
+    %v = "test.secondCodeTrue"() : () -> i32
+    scf.yield %v : i32
+  } else {
+    %v2 = "test.secondCodeFalse"() : () -> i32
+    scf.yield %v2 : i32
+  }
+  return %res : i32
+}
+// CHECK-NEXT:     %[[res:.+]] = scf.if %arg0 -> (i32) {
+// CHECK-NEXT:       "test.firstCodeTrue"() : () -> ()
+// CHECK-NEXT:       %[[tval:.+]] = "test.secondCodeTrue"() : () -> i32
+// CHECK-NEXT:       scf.yield %[[tval]] : i32
+// CHECK-NEXT:     } else {
+// CHECK-NEXT:       "test.firstCodeFalse"() : () -> ()
+// CHECK-NEXT:       %[[fval:.+]] = "test.secondCodeFalse"() : () -> i32
+// CHECK-NEXT:       scf.yield %[[fval]] : i32
+// CHECK-NEXT:     }
+// CHECK-NEXT:     return %[[res]] : i32
+
+
+// CHECK-LABEL: @combineIfs2
+func @combineIfs2(%arg0 : i1, %arg2: i64) -> i32 {
+  scf.if %arg0 {
+    "test.firstCodeTrue"() : () -> ()
+    scf.yield
+  }
+  %res = scf.if %arg0 -> i32 {
+    %v = "test.secondCodeTrue"() : () -> i32
+    scf.yield %v : i32
+  } else {
+    %v2 = "test.secondCodeFalse"() : () -> i32
+    scf.yield %v2 : i32
+  }
+  return %res : i32
+}
+// CHECK-NEXT:     %[[res:.+]] = scf.if %arg0 -> (i32) {
+// CHECK-NEXT:       "test.firstCodeTrue"() : () -> ()
+// CHECK-NEXT:       %[[tval:.+]] = "test.secondCodeTrue"() : () -> i32
+// CHECK-NEXT:       scf.yield %[[tval]] : i32
+// CHECK-NEXT:     } else {
+// CHECK-NEXT:       %[[fval:.+]] = "test.secondCodeFalse"() : () -> i32
+// CHECK-NEXT:       scf.yield %[[fval]] : i32
+// CHECK-NEXT:     }
+// CHECK-NEXT:     return %[[res]] : i32
+
+
+// CHECK-LABEL: @combineIfs3
+func @combineIfs3(%arg0 : i1, %arg2: i64) {
+  scf.if %arg0 {
+    "test.firstCodeTrue"() : () -> ()
+    scf.yield
+  } else {
+    "test.firstCodeFalse"() : () -> ()
+    scf.yield
+  }
+  scf.if %arg0 {
+    "test.secondCodeTrue"() : () -> ()
+    scf.yield
+  }
+  return
+}
+// CHECK-NEXT:     scf.if %arg0 {
+// CHECK-NEXT:       "test.firstCodeTrue"() : () -> ()
+// CHECK-NEXT:       "test.secondCodeTrue"() : () -> ()
+// CHECK-NEXT:     } else {
+// CHECK-NEXT:       "test.firstCodeFalse"() : () -> ()
+// CHECK-NEXT:     }
+
+// CHECK-LABEL: @combineIfs4
+func @combineIfs4(%arg0 : i1, %arg2: i64) {
+  scf.if %arg0 {
+    "test.firstCodeTrue"() : () -> ()
+    scf.yield
+  }
+  scf.if %arg0 {
+    "test.secondCodeTrue"() : () -> ()
+    scf.yield
+  }
+  return
+}
+// CHECK-NEXT:     scf.if %arg0 {
+// CHECK-NEXT:       "test.firstCodeTrue"() : () -> ()
+// CHECK-NEXT:       "test.secondCodeTrue"() : () -> ()
+// CHECK-NEXT:     }