[MLIR][SCF] Sink scf.if from scf.while before region into after region. #165216
Conversation
NexMing
requested review from
Hardcode84,
chelini,
ftynse,
joker-eph,
matthias-springer and
wsmoses
|
@llvm/pr-subscribers-mlir Author: Ming Yan (NexMing) ChangesWhen an scf.if directly precedes an scf.condition in the before region of an scf.while and both share the same condition, move the if into the after region of the loop. This preserves semantics while simplifying the condition region and canonicalizing control flow for further optimizations. Full diff: https://github.com/llvm/llvm-project/pull/165216.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 9bd13f3236cfc..ddf8134d98776 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -3546,6 +3546,137 @@ LogicalResult scf::WhileOp::verify() {
}
namespace {
+/// Move an scf.if op that is directly before the scf.condition op in the while
+/// before region, and whose condition matches the condition of the
+/// scf.condition op, down into the while after region.
+///
+/// scf.while (..) : (...) -> ... {
+/// %additional_used_values = ...
+/// %cond = ...
+/// ...
+/// %res = scf.if %cond -> (...) {
+/// use(%additional_used_values)
+/// ... // then block
+/// scf.yield %then_value
+/// } else {
+/// scf.yield %else_value
+/// }
+/// scf.condition(%cond) %res, ...
+/// } do {
+/// ^bb0(%res_arg, ...):
+/// use(%res_arg)
+/// ...
+///
+/// becomes
+/// scf.while (..) : (...) -> ... {
+/// %additional_used_values = ...
+/// %cond = ...
+/// ...
+/// scf.condition(%cond) %else_value, ..., %additional_used_values
+/// } do {
+/// ^bb0(%res_arg ..., %additional_args): :
+/// use(%additional_args)
+/// ... // if then block
+/// use(%then_value)
+/// ...
+struct WhileMoveIfDown : public OpRewritePattern<WhileOp> {
+ using OpRewritePattern<WhileOp>::OpRewritePattern;
+
+ LogicalResult matchAndRewrite(WhileOp op,
+ PatternRewriter &rewriter) const override {
+ auto conditionOp =
+ cast<scf::ConditionOp>(op.getBeforeBody()->getTerminator());
+ auto ifOp = dyn_cast_or_null<scf::IfOp>(conditionOp->getPrevNode());
+
+ // Check that the ifOp is directly before the conditionOp and that it
+ // matches the condition of the conditionOp. Also ensure that the ifOp has
+ // no else block with content, as that would complicate the transformation.
+ // TODO: support else blocks with content.
+ if (!ifOp || ifOp.getCondition() != conditionOp.getCondition() ||
+ (ifOp.elseBlock() && !ifOp.elseBlock()->without_terminator().empty()))
+ return failure();
+
+ assert(ifOp->use_empty() || (llvm::range_size(ifOp->getUsers()) == 1 &&
+ *ifOp->user_begin() == conditionOp) &&
+ "ifOp has unexpected uses");
+
+ Location loc = op.getLoc();
+
+ // Replace uses of ifOp results in the conditionOp with the yielded values
+ // from the ifOp branches.
+ for (auto [idx, arg] : llvm::enumerate(conditionOp.getArgs())) {
+ auto it = llvm::find(ifOp->getResults(), arg);
+ if (it != ifOp->getResults().end()) {
+ size_t ifOpIdx = it.getIndex();
+ Value thenValue = ifOp.thenYield()->getOperand(ifOpIdx);
+ Value elseValue = ifOp.elseYield()->getOperand(ifOpIdx);
+
+ rewriter.replaceAllUsesWith(it.getBase(), elseValue);
+ rewriter.replaceAllUsesWith(op.getAfterArguments()[idx], thenValue);
+ }
+ }
+
+ SmallVector<Value> additionalUsedValues;
+ auto isValueUsedInsideIf = [&](Value val) {
+ return llvm::any_of(val.getUsers(), [&](Operation *user) {
+ return ifOp.getThenRegion().isAncestor(user->getParentRegion());
+ });
+ };
+
+ // Collect additional used values from before region.
+ for (Operation *it = ifOp->getPrevNode(); it != nullptr;
+ it = it->getPrevNode())
+ llvm::copy_if(it->getResults(), std::back_inserter(additionalUsedValues),
+ isValueUsedInsideIf);
+
+ llvm::copy_if(op.getBeforeArguments(),
+ std::back_inserter(additionalUsedValues),
+ isValueUsedInsideIf);
+
+ // Create new whileOp with additional used values as results.
+ auto additionalValueTypes = llvm::map_to_vector(
+ additionalUsedValues, [](Value val) { return val.getType(); });
+ size_t additionalValueSize = additionalUsedValues.size();
+ SmallVector<Type> newResultTypes(op.getResultTypes());
+ newResultTypes.append(additionalValueTypes);
+
+ auto newWhileOp =
+ scf::WhileOp::create(rewriter, loc, newResultTypes, op.getInits());
+
+ newWhileOp.getBefore().takeBody(op.getBefore());
+ newWhileOp.getAfter().takeBody(op.getAfter());
+ newWhileOp.getAfter().addArguments(
+ additionalValueTypes, SmallVector<Location>(additionalValueSize, loc));
+
+ SmallVector<Value> conditionArgs = conditionOp.getArgs();
+ llvm::append_range(conditionArgs, additionalUsedValues);
+
+ // Update conditionOp inside new whileOp before region.
+ rewriter.setInsertionPoint(conditionOp);
+ rewriter.replaceOpWithNewOp<scf::ConditionOp>(
+ conditionOp, conditionOp.getCondition(), conditionArgs);
+
+ // Replace uses of additional used values inside the ifOp then region with
+ // the whileOp after region arguments.
+ rewriter.replaceUsesWithIf(
+ additionalUsedValues,
+ newWhileOp.getAfterArguments().take_back(additionalValueSize),
+ [&](OpOperand &use) {
+ return ifOp.getThenRegion().isAncestor(
+ use.getOwner()->getParentRegion());
+ });
+
+ // Inline ifOp then region into new whileOp after region.
+ rewriter.eraseOp(ifOp.thenYield());
+ rewriter.inlineBlockBefore(ifOp.thenBlock(), newWhileOp.getAfterBody(),
+ newWhileOp.getAfterBody()->begin());
+ rewriter.eraseOp(ifOp);
+ rewriter.replaceOp(op,
+ newWhileOp->getResults().drop_back(additionalValueSize));
+ return success();
+ }
+};
+
/// Replace uses of the condition within the do block with true, since otherwise
/// the block would not be evaluated.
///
@@ -4258,7 +4389,8 @@ void WhileOp::getCanonicalizationPatterns(RewritePatternSet &results,
results.add<RemoveLoopInvariantArgsFromBeforeBlock,
RemoveLoopInvariantValueYielded, WhileConditionTruth,
WhileCmpCond, WhileUnusedResult, WhileRemoveDuplicatedResults,
- WhileRemoveUnusedArgs, WhileOpAlignBeforeArgs>(context);
+ WhileRemoveUnusedArgs, WhileOpAlignBeforeArgs, WhileMoveIfDown>(
+ context);
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/SCF/canonicalize.mlir b/mlir/test/Dialect/SCF/canonicalize.mlir
index 2bec63672e783..cfae3b34305de 100644
--- a/mlir/test/Dialect/SCF/canonicalize.mlir
+++ b/mlir/test/Dialect/SCF/canonicalize.mlir
@@ -974,6 +974,43 @@ func.func @replace_if_with_cond3(%arg0 : i1, %arg2: i64) -> (i32, i64) {
// -----
+// CHECK-LABEL: @while_move_if_down
+func.func @while_move_if_down() -> i32 {
+ %0 = scf.while () : () -> (i32) {
+ %additional_used_value = "test.get_some_value1" () : () -> (i32)
+ %else_value = "test.get_some_value2" () : () -> (i32)
+ %condition = "test.condition"() : () -> i1
+ %res = scf.if %condition -> (i32) {
+ "test.use1" (%additional_used_value) : (i32) -> ()
+ %then_value = "test.get_some_value3" () : () -> (i32)
+ scf.yield %then_value : i32
+ } else {
+ scf.yield %else_value : i32
+ }
+ scf.condition(%condition) %res : i32
+ } do {
+ ^bb0(%res_arg: i32):
+ "test.use2" (%res_arg) : (i32) -> ()
+ scf.yield
+ }
+ return %0 : i32
+}
+// CHECK-NEXT: %[[WHILE_0:.*]]:2 = scf.while : () -> (i32, i32) {
+// CHECK-NEXT: %[[VAL_0:.*]] = "test.get_some_value1"() : () -> i32
+// CHECK-NEXT: %[[VAL_1:.*]] = "test.get_some_value2"() : () -> i32
+// CHECK-NEXT: %[[VAL_2:.*]] = "test.condition"() : () -> i1
+// CHECK-NEXT: scf.condition(%[[VAL_2]]) %[[VAL_1]], %[[VAL_0]] : i32, i32
+// CHECK-NEXT: } do {
+// CHECK-NEXT: ^bb0(%[[VAL_3:.*]]: i32, %[[VAL_4:.*]]: i32):
+// CHECK-NEXT: "test.use1"(%[[VAL_4]]) : (i32) -> ()
+// CHECK-NEXT: %[[VAL_5:.*]] = "test.get_some_value3"() : () -> i32
+// CHECK-NEXT: "test.use2"(%[[VAL_5]]) : (i32) -> ()
+// CHECK-NEXT: scf.yield
+// CHECK-NEXT: }
+// CHECK-NEXT: return %[[VAL_6:.*]]#0 : i32
+
+// -----
+
// CHECK-LABEL: @while_cond_true
func.func @while_cond_true() -> i1 {
%0 = scf.while () : () -> i1 {
|
|
@llvm/pr-subscribers-mlir-scf Author: Ming Yan (NexMing) ChangesWhen an scf.if directly precedes an scf.condition in the before region of an scf.while and both share the same condition, move the if into the after region of the loop. This preserves semantics while simplifying the condition region and canonicalizing control flow for further optimizations. Full diff: https://github.com/llvm/llvm-project/pull/165216.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 9bd13f3236cfc..ddf8134d98776 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -3546,6 +3546,137 @@ LogicalResult scf::WhileOp::verify() {
}
namespace {
+/// Move an scf.if op that is directly before the scf.condition op in the while
+/// before region, and whose condition matches the condition of the
+/// scf.condition op, down into the while after region.
+///
+/// scf.while (..) : (...) -> ... {
+/// %additional_used_values = ...
+/// %cond = ...
+/// ...
+/// %res = scf.if %cond -> (...) {
+/// use(%additional_used_values)
+/// ... // then block
+/// scf.yield %then_value
+/// } else {
+/// scf.yield %else_value
+/// }
+/// scf.condition(%cond) %res, ...
+/// } do {
+/// ^bb0(%res_arg, ...):
+/// use(%res_arg)
+/// ...
+///
+/// becomes
+/// scf.while (..) : (...) -> ... {
+/// %additional_used_values = ...
+/// %cond = ...
+/// ...
+/// scf.condition(%cond) %else_value, ..., %additional_used_values
+/// } do {
+/// ^bb0(%res_arg ..., %additional_args): :
+/// use(%additional_args)
+/// ... // if then block
+/// use(%then_value)
+/// ...
+struct WhileMoveIfDown : public OpRewritePattern<WhileOp> {
+ using OpRewritePattern<WhileOp>::OpRewritePattern;
+
+ LogicalResult matchAndRewrite(WhileOp op,
+ PatternRewriter &rewriter) const override {
+ auto conditionOp =
+ cast<scf::ConditionOp>(op.getBeforeBody()->getTerminator());
+ auto ifOp = dyn_cast_or_null<scf::IfOp>(conditionOp->getPrevNode());
+
+ // Check that the ifOp is directly before the conditionOp and that it
+ // matches the condition of the conditionOp. Also ensure that the ifOp has
+ // no else block with content, as that would complicate the transformation.
+ // TODO: support else blocks with content.
+ if (!ifOp || ifOp.getCondition() != conditionOp.getCondition() ||
+ (ifOp.elseBlock() && !ifOp.elseBlock()->without_terminator().empty()))
+ return failure();
+
+ assert(ifOp->use_empty() || (llvm::range_size(ifOp->getUsers()) == 1 &&
+ *ifOp->user_begin() == conditionOp) &&
+ "ifOp has unexpected uses");
+
+ Location loc = op.getLoc();
+
+ // Replace uses of ifOp results in the conditionOp with the yielded values
+ // from the ifOp branches.
+ for (auto [idx, arg] : llvm::enumerate(conditionOp.getArgs())) {
+ auto it = llvm::find(ifOp->getResults(), arg);
+ if (it != ifOp->getResults().end()) {
+ size_t ifOpIdx = it.getIndex();
+ Value thenValue = ifOp.thenYield()->getOperand(ifOpIdx);
+ Value elseValue = ifOp.elseYield()->getOperand(ifOpIdx);
+
+ rewriter.replaceAllUsesWith(it.getBase(), elseValue);
+ rewriter.replaceAllUsesWith(op.getAfterArguments()[idx], thenValue);
+ }
+ }
+
+ SmallVector<Value> additionalUsedValues;
+ auto isValueUsedInsideIf = [&](Value val) {
+ return llvm::any_of(val.getUsers(), [&](Operation *user) {
+ return ifOp.getThenRegion().isAncestor(user->getParentRegion());
+ });
+ };
+
+ // Collect additional used values from before region.
+ for (Operation *it = ifOp->getPrevNode(); it != nullptr;
+ it = it->getPrevNode())
+ llvm::copy_if(it->getResults(), std::back_inserter(additionalUsedValues),
+ isValueUsedInsideIf);
+
+ llvm::copy_if(op.getBeforeArguments(),
+ std::back_inserter(additionalUsedValues),
+ isValueUsedInsideIf);
+
+ // Create new whileOp with additional used values as results.
+ auto additionalValueTypes = llvm::map_to_vector(
+ additionalUsedValues, [](Value val) { return val.getType(); });
+ size_t additionalValueSize = additionalUsedValues.size();
+ SmallVector<Type> newResultTypes(op.getResultTypes());
+ newResultTypes.append(additionalValueTypes);
+
+ auto newWhileOp =
+ scf::WhileOp::create(rewriter, loc, newResultTypes, op.getInits());
+
+ newWhileOp.getBefore().takeBody(op.getBefore());
+ newWhileOp.getAfter().takeBody(op.getAfter());
+ newWhileOp.getAfter().addArguments(
+ additionalValueTypes, SmallVector<Location>(additionalValueSize, loc));
+
+ SmallVector<Value> conditionArgs = conditionOp.getArgs();
+ llvm::append_range(conditionArgs, additionalUsedValues);
+
+ // Update conditionOp inside new whileOp before region.
+ rewriter.setInsertionPoint(conditionOp);
+ rewriter.replaceOpWithNewOp<scf::ConditionOp>(
+ conditionOp, conditionOp.getCondition(), conditionArgs);
+
+ // Replace uses of additional used values inside the ifOp then region with
+ // the whileOp after region arguments.
+ rewriter.replaceUsesWithIf(
+ additionalUsedValues,
+ newWhileOp.getAfterArguments().take_back(additionalValueSize),
+ [&](OpOperand &use) {
+ return ifOp.getThenRegion().isAncestor(
+ use.getOwner()->getParentRegion());
+ });
+
+ // Inline ifOp then region into new whileOp after region.
+ rewriter.eraseOp(ifOp.thenYield());
+ rewriter.inlineBlockBefore(ifOp.thenBlock(), newWhileOp.getAfterBody(),
+ newWhileOp.getAfterBody()->begin());
+ rewriter.eraseOp(ifOp);
+ rewriter.replaceOp(op,
+ newWhileOp->getResults().drop_back(additionalValueSize));
+ return success();
+ }
+};
+
/// Replace uses of the condition within the do block with true, since otherwise
/// the block would not be evaluated.
///
@@ -4258,7 +4389,8 @@ void WhileOp::getCanonicalizationPatterns(RewritePatternSet &results,
results.add<RemoveLoopInvariantArgsFromBeforeBlock,
RemoveLoopInvariantValueYielded, WhileConditionTruth,
WhileCmpCond, WhileUnusedResult, WhileRemoveDuplicatedResults,
- WhileRemoveUnusedArgs, WhileOpAlignBeforeArgs>(context);
+ WhileRemoveUnusedArgs, WhileOpAlignBeforeArgs, WhileMoveIfDown>(
+ context);
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/SCF/canonicalize.mlir b/mlir/test/Dialect/SCF/canonicalize.mlir
index 2bec63672e783..cfae3b34305de 100644
--- a/mlir/test/Dialect/SCF/canonicalize.mlir
+++ b/mlir/test/Dialect/SCF/canonicalize.mlir
@@ -974,6 +974,43 @@ func.func @replace_if_with_cond3(%arg0 : i1, %arg2: i64) -> (i32, i64) {
// -----
+// CHECK-LABEL: @while_move_if_down
+func.func @while_move_if_down() -> i32 {
+ %0 = scf.while () : () -> (i32) {
+ %additional_used_value = "test.get_some_value1" () : () -> (i32)
+ %else_value = "test.get_some_value2" () : () -> (i32)
+ %condition = "test.condition"() : () -> i1
+ %res = scf.if %condition -> (i32) {
+ "test.use1" (%additional_used_value) : (i32) -> ()
+ %then_value = "test.get_some_value3" () : () -> (i32)
+ scf.yield %then_value : i32
+ } else {
+ scf.yield %else_value : i32
+ }
+ scf.condition(%condition) %res : i32
+ } do {
+ ^bb0(%res_arg: i32):
+ "test.use2" (%res_arg) : (i32) -> ()
+ scf.yield
+ }
+ return %0 : i32
+}
+// CHECK-NEXT: %[[WHILE_0:.*]]:2 = scf.while : () -> (i32, i32) {
+// CHECK-NEXT: %[[VAL_0:.*]] = "test.get_some_value1"() : () -> i32
+// CHECK-NEXT: %[[VAL_1:.*]] = "test.get_some_value2"() : () -> i32
+// CHECK-NEXT: %[[VAL_2:.*]] = "test.condition"() : () -> i1
+// CHECK-NEXT: scf.condition(%[[VAL_2]]) %[[VAL_1]], %[[VAL_0]] : i32, i32
+// CHECK-NEXT: } do {
+// CHECK-NEXT: ^bb0(%[[VAL_3:.*]]: i32, %[[VAL_4:.*]]: i32):
+// CHECK-NEXT: "test.use1"(%[[VAL_4]]) : (i32) -> ()
+// CHECK-NEXT: %[[VAL_5:.*]] = "test.get_some_value3"() : () -> i32
+// CHECK-NEXT: "test.use2"(%[[VAL_5]]) : (i32) -> ()
+// CHECK-NEXT: scf.yield
+// CHECK-NEXT: }
+// CHECK-NEXT: return %[[VAL_6:.*]]#0 : i32
+
+// -----
+
// CHECK-LABEL: @while_cond_true
func.func @while_cond_true() -> i1 {
%0 = scf.while () : () -> i1 {
|
NexMing
force-pushed
the
dev/while-sink-if-down
branch
from
4d3ff6d to
ecffe33
Compare
MaheshRavishankar
left a comment
MaheshRavishankar
left a comment
There was a problem hiding this comment.
I am not sure this is a canonicalization. This seems like a cost-model based thing. Someone else might want to hoist the if condition
matthias-springer
left a comment
matthias-springer
left a comment
There was a problem hiding this comment.
This preserves semantics while simplifying the condition region and canonicalizing control flow for further optimizations.
What further optimizations did you have in mind?
The motivation is that I want to lift When I use the When I continued using I believe this optimization always simplifies the control flow, so I think it belongs to canonicalization. |
|
ping |
It would be good to not have this as a canonicalization to start with. This seems like a pretty heavy weight transformation to add to canonicalizations. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
NexMing
force-pushed
the
dev/while-sink-if-down
branch
from
83f42de to
05b6a0f
Compare
NexMing
force-pushed
the
dev/while-sink-if-down
branch
from
05b6a0f to
19a042e
Compare
|
Please help review it, thank you. @MaheshRavishankar |
MaheshRavishankar
dismissed
their stale review
Removing blockers since this isnt a canonicalization anymore.
Which part? The transformation itself? As long as the matching phase has a "fail fast" path it seems fine to me actually. |
An if-hoisting across a while does not fit in canonicalizations. |
|
I think we need to find a way out of this circular discussion. The onus is to prove that this is canonical. Just stating "I can't think of why you wouldn't do it" isn't justification enough. You can't turn that around and make it an onus on reviewers as to prove why this shouldnt be a canonicalization. Of all possible representations, you need to prove why a canonical representation is always superior. But in any case, the pattern is doing a whole bunch of code motion and moving instructions around. That is an extremely heavy weight transformation. This is not a targeted "get to a better representation of an operation" transformation. |
Sorry but at some point the onus is on you to come up with principled arguments, otherwise I can't really take your objections seriously. Canonicalization can't be defined as "whatever Mahesh think is tasteful".
This is not how it works, it has never been the bar, and it won't be. I believe I explained this multiple times, the basic example of pushing constant to the right hand side defeats already any "proving" bar you're trying to put out. |
So please tell me what are the principles arguments for what should be a canonicalization. I think if you can enunciate that well, then we can discuss it better. It is not about my taste. This pattern is doing major code movement. Please explain to me the framework for canonicalization that justifies doing code movement this way. I have provided an actual concrete reason why this shouldnt be a canonicalization in my opinion (not a matter of taste). Now can you provide a counter argument as to why this should be a canonicalization. Anyway, I think this is not on the author of this change. The change as is fine and well structured in my book. |
MaheshRavishankar
left a comment
MaheshRavishankar
left a comment
There was a problem hiding this comment.
Oh, I see this was moved back into canonicalization. So I was mistaken on the state of the change.
Really this is bad practice! I urge you to not add it as a canonicalization, but I dont have the heart to block this change, even though in my book this is following bad practices.
I believe I did, multiple times, but I can re-iterate. It turns out we actually have a documentation here: https://mlir.llvm.org/docs/Canonicalization/ ; in particular it links to an pretty good an article on the topic.
You indeed elaborate a little bit in your previous message, thanks for this. That said it feels a bit fuzzy to me still, let me quote the only concrete sentence:
You're here repeating the use of "heavy weight transformation" which is ambiguous to me. I understand it as "expensive" in terms of compile-time, which is why I asked before:
But you didn't specifically followed-up on this. So now that you're prefixing it with "doing a whole bunch of code motion and moving instructions around", I suspect you may intended to address something about the scope of the transformation to talk about "heavyweight"?
Sure:
|
|
I have tried to describe the issue I have with canonicalization too, most notably here https://discourse.llvm.org/t/rfc-update-to-general-design-section-of-operation-canonicalizations-in-mlir/79355. But let me try to be more concrete, and maybe we need to move this to that old (or new) discourse post, but here is what canonicalizations should do afaics, a lot of which is driven by the fact that canonicalizations are applied everywhere in a given compiler stack and provide almost no control to users, which is the anti-pattern.
That btw, is exactly what I have said before here https://discourse.llvm.org/t/rfc-update-to-general-design-section-of-operation-canonicalizations-in-mlir/79355/31?u=maheshravishankar . I dont know how else to state it. As it stands, the proclivity of everyone just adding to canonicalization without justification or analysis is a bad practice being promoted which is ultimately going to be a major issue for anyone building a serious compiler stack using MLIR. Specific to this example, to me the movement of operations from the |
I’ve already moved it out of canonicalization; now it’s in I’m not opposed to moving it out of canonicalization, but I believe the reason for temporarily moving it out of canonicalization is that this transformation is complex and expensive, which introduces instability and uncertainty., rather than it being unsuitable for canonicalization. Once this optimization becomes sufficiently stable, could we move it back into the canonicalization stage? Or is my understanding of canonicalization too broad? Perhaps this transformation belongs in something like a “simplify CFG” pass instead. I would prefer to decide this based on heuristics rather than strict definitions and proofs, because it’s difficult to guarantee that something is beneficial in every case. There will inevitably be trade-offs. |
Sure, I just plainly disagree with your approach to it.
You seem to put what seems to me to be an impossible bar to reach for any practical development in LLVM/MLIR historically speaking: I don't see how anyone can "prove" this. The system isn't defined in terms of mathematical lattice that we can reason about, a lot of it is ad-hoc engineering: this is obviously imperfect but this is what we have to work with.
I don't quite see what conclusion you're trying to make from this observation, I can imagine many root causes which have nothing to do with what you're trying to convey here.
If this is what you believe, fine: just stop using canonicalization! Nobody forces you to bring these into your compiler.
As far as I understand, this transformation is doing: -> I don't quite get why we'd need any side-effect analysis here, this is a "simple" simplification of redundant control-flow (I even wonder if the
That's totally not the kind of things I would consider. First it's highly dependent on the particular pass pipeline and the shape of the IR, but also that would preclude a lot of obvious canonicalization, starting with folding |
If there is "instability and uncertainty", that's a problem to add this in the first place. |
As far as I understand, this transformation is doing: I think this may be over simplifying the transformation a bit. Maybe I'm wrong (so feel free to correct my example if i'm wrong), but let me try to give an example. Let's take the case: If you decide to do the same transformation, you need to somehow also forward
Both of these seem heavy weight. Note that you cannot really safely do this transformation without doing atleast one of these. Analyze a whole region or cloning an entire backward chain (or finding that backward chain) seems a bit too much for a canonicalization maybe. |
What I’m currently worried about is an extreme case where the number of operands to |
That is valid concern. I think you are saying appearance to also mean ability to analyze. It could result in more complicated liveness analysis than pre-transform (and to Kumar's point, trade off between recomputing or increasing high water mark). As a general pattern though that is something that could be analyzed in different flows. It seems desirable/useful pattern but not always on/non canonical, so currently proposed as pattern seems sensible. We don't have a big corpus upstream to measure cost, so I don't think we can put that bar here (of course if folks can find cases where bad and file issues great). It seems useful pattern that can be further refined here. |
7 participants
When a
scf.ifdirectly precedes an scf.condition in the before region of an scf.while and both share the same condition, move the if into the after region of the loop. This helps simplify the control flow to enable upliftingscf.whiletoscf.for.