-
Notifications
You must be signed in to change notification settings - Fork 16.1k
[mlir][SCF] Fix region branch op interfaces for scf.forall and its terminator
#174221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -649,13 +649,6 @@ def ForallOp : SCF_Op<"forall", [ | |
|
|
||
| /// Returns true if the mapping specified for this forall op is linear. | ||
| bool usesLinearMapping(); | ||
|
|
||
| /// RegionBranchOpInterface | ||
|
|
||
| OperandRange getEntrySuccessorOperands(RegionSuccessor successor) { | ||
| return getInits(); | ||
| } | ||
|
|
||
| }]; | ||
| } | ||
|
|
||
|
|
@@ -667,6 +660,7 @@ def InParallelOp : SCF_Op<"forall.in_parallel", [ | |
| Pure, | ||
| Terminator, | ||
| DeclareOpInterfaceMethods<InParallelOpInterface>, | ||
| ReturnLike, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This is a shortcut for |
||
| HasParent<"ForallOp">, | ||
| ] # GraphRegionNoTerminator.traits> { | ||
| let summary = "terminates a `forall` block"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2013,21 +2013,26 @@ void ForallOp::getCanonicalizationPatterns(RewritePatternSet &results, | |
| ForallOpReplaceConstantInductionVar>(context); | ||
| } | ||
|
|
||
| /// Given the region at `index`, or the parent operation if `index` is None, | ||
| /// return the successor regions. These are the regions that may be selected | ||
| /// during the flow of control. `operands` is a set of optional attributes that | ||
| /// correspond to a constant value for each operand, or null if that operand is | ||
| /// not a constant. | ||
| void ForallOp::getSuccessorRegions(RegionBranchPoint point, | ||
| SmallVectorImpl<RegionSuccessor> ®ions) { | ||
| // In accordance with the semantics of forall, its body is executed in | ||
| // parallel by multiple threads. We should not expect to branch back into | ||
| // the forall body after the region's execution is complete. | ||
| if (point.isParent()) | ||
| regions.push_back(RegionSuccessor(&getRegion(), getRegionIterArgs())); | ||
| else | ||
| regions.push_back( | ||
| RegionSuccessor(getOperation(), getOperation()->getResults())); | ||
| // There are two region branch points: | ||
| // 1. "parent": entering the forall op for the first time. | ||
| // 2. scf.in_parallel terminator | ||
| if (point.isParent()) { | ||
| // When first entering the forall op, the control flow typically branches | ||
| // into the forall body. (In parallel for multiple threads.) | ||
| regions.push_back(RegionSuccessor(&getRegion())); | ||
| // However, when there are 0 threads, the control flow may branch back to | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we test the 0 thread case?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I updated the test case to use an SSA value for the number of threads. In that case, we must populate both region successors because we do not statically know the branching behavior. Is that good enough? (That test case used to produce incorrect results before.) |
||
| // the parent immediately. | ||
| regions.emplace_back(getOperation(), | ||
| ResultRange{getResults().end(), getResults().end()}); | ||
| } else { | ||
| // In accordance with the semantics of forall, its body is executed in | ||
| // parallel by multiple threads. We should not expect to branch back into | ||
| // the forall body after the region's execution is complete. | ||
| regions.emplace_back(getOperation(), | ||
| ResultRange{getResults().end(), getResults().end()}); | ||
| } | ||
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This now falls back to the default implementation of
getEntrySuccessorOperands: an empty operand range.