- Notifications
You must be signed in to change notification settings - Fork14.5k
[mlir][tosa] Check for isolated regions intosa.while_loop#144865
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This commit fixes a check in the validation pass which intendedto validate whether a `tosa.cond_if` operation was conformant tothe specification. The specification requires all values used inthe then/else regions are explicitly declared within the regions.This change checks that these regions are 'isolated from above',to ensure this requirement is true.Change-Id: I1b6eac1ed571e6b1eda4a58f0677c80e22977e58
Similarly to `tosa.cond_if`, this patch checks that the cond/bodyregions of `tosa.while_loop` are isolated from above. This is requiredsince the specification requires all values used in the cond/bodyregions are explicitly declared within the regions.Change-Id: Ia7396b9811db54805ec33befd24ab97d1b605905
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.
LGTM % a few pedantic comments :D
| bool isElseGraphNullaryRegion = isNullaryRegion(elseGraph); | ||
| bool isInputListEmpty = ifOp.getInputList().size() == 0; | ||
| bool checkErrorIfWhileLoop(Operation *op) { | ||
| auto whileOp = dyn_cast<tosa::WhileOp>(op); |
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.
Could this beconst?
| if ((isInputListEmpty != isThenGraphNullaryRegion) || | ||
| (isInputListEmpty != isElseGraphNullaryRegion)) { | ||
| Region &condGraph = whileOp.getCondGraph(); |
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.
const?
| (isInputListEmpty != isElseGraphNullaryRegion)) { | ||
| Region &condGraph = whileOp.getCondGraph(); | ||
| Region &bodyGraph = whileOp.getBodyGraph(); | ||
| bool isCondGraphIsolatedRegion = isRegionIsolatedFromAbove(condGraph); |
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.
const?
| if ((isInputListEmpty != isThenGraphNullaryRegion) || | ||
| (isInputListEmpty != isElseGraphNullaryRegion)) { | ||
| Region &condGraph = whileOp.getCondGraph(); | ||
| Region &bodyGraph = whileOp.getBodyGraph(); |
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.
const?
| Region &condGraph = whileOp.getCondGraph(); | ||
| Region &bodyGraph = whileOp.getBodyGraph(); | ||
| bool isCondGraphIsolatedRegion = isRegionIsolatedFromAbove(condGraph); | ||
| bool isBodyGraphIsolatedRegion = isRegionIsolatedFromAbove(bodyGraph); |
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.
const?
| op->emitOpError() | ||
| << "the current simplified form is not strictly conformant to the " | ||
| "spec, please use the generic format\n"; | ||
| << "is not conformant to the TOSA specification. It requires the " |
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.
Similarly to theif case, we could provide more information here if we were to split the checks into two. Only a week preference for this though.
| %0 = "tosa.const"() {values = dense<0> : tensor<i32>} : () -> tensor<i32> | ||
| // expected-error@+1 {{'tosa.while_loop' op is not conformant to the TOSA specification. It requires the cond/body regions are isolated from above.}} | ||
| %1 = "tosa.while_loop"(%0) ({ | ||
| ^bb0(%arg3: tensor<i32>): |
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.
Should the blocks have the same IDbb0?
| func.func @test_while_loop_isolated_from_above(%arg0: tensor<f32>, %arg1: tensor<i32>) { | ||
| %0 = "tosa.const"() {values = dense<0> : tensor<i32>} : () -> tensor<i32> | ||
| %1:3 = "tosa.while_loop"(%0, %arg0, %arg1) ({ | ||
| ^bb0(%arg3: tensor<i32>, %arg4: tensor<f32>, %arg5: tensor<i32>): |
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.
Should the blocks have the same IDbb0?
| // ----- | ||
| // COM: Check isolated while_loops are valid |
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.
Do we need to use theCOM directive here if there are no strings in the comment that could be interpreted as a FileCheck directive?
| %3 = "tosa.logical_not"(%2) : (tensor<i1>) -> tensor<i1> | ||
| tosa.yield %3 : tensor<i1> | ||
| }, { | ||
| ^bb0(%arg3: tensor<i32>): |
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.
Here it looks like the body graph is isolated from above, and it is only the condition graph that isn't. Should we add another test to check the error is triggered when the body graph isn't isolated from above?
| isa<tosa::YieldOp>(op) || isa<tosa::VariableOp>(op)) | ||
| return true; | ||
| return false; | ||
| static bool isOpIsolatedFromAbove(Operation *op, Region ®ion) { |
udaya-rangaJul 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Will there just be 1 copy after merging this and the other review forcond.if or is this patch built on top of the other so that it appears here as well ? I guess the latter ?
Similarly to
tosa.cond_if, this patch checks that the cond/body regions oftosa.while_loopare isolated from above. This is required since the specification requires all values used in the cond/body regions are explicitly declared within the regions.Note: this change is dependent on#143772