[MLIR][SCF] Sink scf.if from scf.while before region into after region in scf-uplift-while-to-for #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. |
|
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. |
…it into canonicalization. We do not have an appropriate cost model for evaluation, and adding more constraints can ensure that it is always beneficial and does not introduce excessive compile-time overhead.
|
Ping! |
🐧 Linux x64 Test Results
|
mlir/lib/Dialect/SCF/IR/SCF.cpp
Outdated
There was a problem hiding this comment.
Why this restriction on the results and its use here? In particular what is the hasNUses(2) meant to cover?
This all seems very ad-hoc right now: you're matching something that seems quite overly specific to me.
There was a problem hiding this comment.
Because values defined in before region cannot reach the do region, I need to ensure that the cond value is not used inside scf.if (although this usually doesn’t happen). Otherwise, I would need to pass or create new values to update the use-chain.
For simplicity, I restrict the cond value to have exactly two uses (these two uses come from the scf.if and scf.condition operations).
There was a problem hiding this comment.
I reiterate my previous comment/concern: to me that makes it really overly specific in an arbitrary way right now.
There was a problem hiding this comment.
My intention is mainly to cover the scf.while form produced by the lift-cf-to-scf pass. Making the pattern more specific helps us avoid additional analysis, which reduces compile-time overhead.
There was a problem hiding this comment.
I understand, but my remark still stands: that is quite overly specific right now, for reasons I don't quite grasp.
There was a problem hiding this comment.
I find it hard to accept the statement. Does this mean that your transformations may also produce dead values that cannot be eliminated?
Have you considered extreme cases or stress tests? Or could this actually be a hidden optimization bug that simply hasn’t been discovered yet?
Subjectively, I feel that adding result values to scf.while may introduce negative optimization effects. Maybe I just don’t understand it well enough — can you explain why it wouldn’t cause any negative optimization?
I don't see any issue with what you call "dead values" here. But let's not reverse the burden of explanation: if you claim that this is a problem, it's up to you to provide an example (you just need one), not the opposite actually (I can't provide an example for the absence of something in general...).
If I can guarantee that it always results in a positive optimization, then I think it’s fine to use it in canonicalization.
I was trying to provide you with a way to make progress right now to land your change, I suspect we may not converge on this PR right now.
There was a problem hiding this comment.
I don't see any issue with what you call "dead values" here. But let's not reverse the burden of explanation: if you claim that this is a problem, it's up to you to provide an example (you just need one), not the opposite actually (I can't provide an example for the absence of something in general...).
I believe that the "seems like the kind of usual transformations we do." as you referred to, generally do not generate dead values that cannot be eliminated.
in the following example
%res = scf.while {
a large number of defined values...
scf.if %cond {
used a large number of values...
}
scf.condition %cond
} do {
...
The converted scf.while defines a large number of values, but most of them have no users, and it seems that other optimizations can hardly eliminate them.
%res:xxxx = scf.while {
a large number of defined values...
scf.condition %cond (carrying a large number of operands)
} do {
used a large number of values...
...
This is my subjective guess that such a transformation might introduce optimization risks (it’s just a guess... and I believe I’m not the only one who would think so). You can question my guess, and you can also explain why there is no risk. But since there has been no reasonable explanation, it leaves me very confused...
There was a problem hiding this comment.
I was trying to provide you with a way to make progress right now to land your change, I suspect we may not converge on this PR right now.
I’ve already moved it into test-scf-uplift-while-to-for. Could you please help make progress this PR? Thank you very much.
There was a problem hiding this comment.
You can question my guess, and you can also explain why there is no risk.
As mentioned before It's very difficult to prove or explain the absence of a problem when there is none. It's very easy to provide an example of a problem when it exists. So the onus is on you here to sustain your "guess".
and it seems that other optimizations can hardly eliminate them.
You're introducing the concept of "dead values" here, I don't have a problem with these values existing. If you want to argue something you need to look at whether it affects liveness and live ranges, or how it is treated in a lowering pipeline: what happens with such a while loop when lowered to a CFG? Hopefully the logic is already smart enough to ignore these values, but it's possible we can do better (I don't see any fundamental issue here).
There was a problem hiding this comment.
You're introducing the concept of "dead values" here, I don't have a problem with these values existing. If you want to argue something you need to look at whether it affects liveness and live ranges, or how it is treated in a lowering pipeline: what happens with such a while loop when lowered to a CFG? Hopefully the logic is already smart enough to ignore these values, but it's possible we can do better (I don't see any fundamental issue here).
Hi, I tried to see what happens when scf.while is lowered to the CFG.
I found that when scf.condition is lowered to cf.cond_br, the “dead values” disappear. So my earlier guess was incorrect, and it doesn’t introduce optimization risks. I apologize for my mistaken "guess". I think I’ll submit a new PR and continue using the original approach. Sorry for any trouble this may have caused you.
But I do have a question: why isn’t scf.condition designed with two sets of operands—one passed to the do region and the other passed as the result values?
Groverkss
dismissed
their stale review
on vacation and don't have strong concerns here
NexMing
force-pushed
the
dev/while-sink-if-down
branch
from
6debfbf to
a289930
Compare
|
ping |
joker-eph
changed the title
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/17/builds/13018 Here is the relevant piece of the build log for the reference |
…er region in scf-uplift-while-to-for" (llvm#169888) Reverts llvm#165216 It is implemented in llvm#169892 .
…on into after region in scf-uplift-while-to-for" (#169888) Reverts llvm/llvm-project#165216 It is implemented in #169892 .
…n in scf-uplift-while-to-for (llvm#165216) When a `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 helps simplify the control flow to enable uplifting `scf.while` to `scf.for`.
…er region in scf-uplift-while-to-for" (llvm#169888) Reverts llvm#165216 It is implemented in llvm#169892 .
…n in scf-uplift-while-to-for (llvm#165216) When a `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 helps simplify the control flow to enable uplifting `scf.while` to `scf.for`.
…er region in scf-uplift-while-to-for" (llvm#169888) Reverts llvm#165216 It is implemented in llvm#169892 .
…er region in scf-uplift-while-to-for" (llvm#169888) Reverts llvm#165216 It is implemented in llvm#169892 .
8 participants
When a
scf.ifdirectly precedes anscf.conditionin the before region of anscf.whileand 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.