Follows up on suggested addition deferred in initial TOSA push
Details
Diff Detail
Event Timeline
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
22 ↗ | (On Diff #309370) | I just noticed here, but why is there explicit capture for this instead of implicit capture? (similarly, I'd expect the while to not be isolated either) |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
22 ↗ | (On Diff #309370) | I don’t think we’ve a particular preference here. Does MLIR suggest implicit capture for better codegen ? From what I recall, legalization from TF/TFLite generated explicit capture. Perhaps this was an artifact of the constructs used then. Is there a flag that controls this ? Meanwhile I've updated the tests. |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
14 ↗ | (On Diff #309431) | I don't quite get what you're testing here? |
48 ↗ | (On Diff #309431) | There are no check lines here. Also this file is about constant folding, and I can see how a conditional with a constant condition can be folded, but is this something we get out of the branch interface? |
22 ↗ | (On Diff #309370) | Implicit capture is more friendly to manipulate in general for these op. TensorFlow while/if op with regions operate this way right now, scf.if and scf.while as well. |
mlir/lib/Dialect/Tosa/IR/TosaOps.cpp | ||
---|---|---|
90 | Is there a reason why you need to 'this->' qualify this expression? Possibly rename 'elseBranch' to 'optionalElseBranch' to make it more clear why you are creating this alias? (I believe this was mostly copied from scf where the accessor is named elseBranch(), thereby requiring a disambiguation. Distinct naming would help and eliminate the need) | |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
14 ↗ | (On Diff #309431) | It looks like you are testing constant folding. I would prefer a positive check of the actual result of that vs CHECK-NOT. Also, please verify both true (1), false (0) and non constant. |
22 ↗ | (On Diff #309370) | +1 to implicit capture |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
14 ↗ | (On Diff #309431) | Adding true, false and non-constant cases. More detail below. |
48 ↗ | (On Diff #309431) | The genesis of this patch is a suggestion from rriddle in the original TOSA dialect contribution to add multiple new interfaces to the original code including RecursiveSideEffects and RegionBranchOpInterface (this one), and we were advised that they aid multiple underlying MLIR features including constant folding and constant propagation. These are new to us. Admittedly we are flying a little blind here, since we pretty much added interfaces as advised. There appear no targeted tests for these in SCF or any other dialect to guide us. The TensorFlow dialects do not apparently utilize this interface. Given that these are standard interfaces, should each consuming dialect subsume their testing separately, or can this be done in a single place, e.g. SCF ? As described in an earlier comment, the TOSA constructs for this interface are derived from the SCF one, since we followed that dialect while adding this interface as suggested. |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
48 ↗ | (On Diff #309431) | SCCP will use this interface to do constant folding across region control flow. Many other analyses/transforms will also use it to do proper dataflow analysis. I would only omit it if you don't intend to use the TOSA dialect for useful analysis/transformation(which I expect is the intent). If you wanted to test the interface, you could write a test for one of the components that use it. For constant folding, that would be checking that SCCP(sccp) properly propagates constants through the regions. |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
48 ↗ | (On Diff #309431) | We'd certainly like to keep these interfaces within the TOSA dialect - any interfaces that aid analysis and codegen are worth having. Primarily, we're concerned we're overloading the effort of having what seems like general interface validation tests onto this dialect. Our unfamiliarity with all the capabilities of the interface makes it harder to write general validation tests for it - which shows in the questions related to the tests here. Would it be more effective if tests for this interface could be expressed in the context of SCF and we adapt them for TOSA too, as required ? |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
48 ↗ | (On Diff #309431) | We're not looking to test the interface itself, but to test that the implementation of the interface on the TOSA dialect is correct and hooked up properly. |
mlir/test/Dialect/Tosa/constant_folding.mlir | ||
---|---|---|
48 ↗ | (On Diff #309431) | My litmus test for any code change is to make sure that there is a test that would fail if I remove your code change. |
I've attempted to reconstruct the tests with insights from the sccp tests. While doing so, I restricted this interface to cond_if() alone to simplify this effort. I've also changed the conditional of the op to just a boolean I1 rather than an unnecessary I1Tensor, allowing the check in getSuccessorRegions() to work without modification.
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td | ||
---|---|---|
1648 | Followed SCF expression of conditional if operation. From what we understand, this reflects the else block being optional. Would it be better to use SizedRegion here too ? | |
mlir/test/Dialect/Tosa/sccp.mlir | ||
1 | I assume you mean split-input-file ? Sure we can. We picked it up when we looked at the sccp test targeting the built-in dialect. | |
42 | This follows the checks we saw in the tests/Transforms/sccp.mlir . We're trying to validate that with a constant conditional, the compile time evaluated path receives a particular predefined constant value. |
mlir/test/Dialect/Tosa/sccp.mlir | ||
---|---|---|
1 | Sorry I meant allow-unregistered-dialect (it should highlight it if you mouse over) | |
42 | But your CHECKS aren't doing this: try to remove -pass-pipeline="func(sccp)" and the test will be passing. I'll go back to my previous comment:
|
At this point the best option appear to be to defer this change. Let's revisit this when the SCF dialect has corresponding tests for this interface. Right now, we aren't able to effectively exercise it, and lack access to precedent material on this interface that we can effectively utilize to construct tests, and don't have familiarity with this domain of MLIR. Thank you for your feedback!
Aside from general MLIR coding style, this is not really an area of expertise for me, so I defer to Mehdi/River for feedback. I do feel like it might be a bit easier to see how all of this fits together a little bit down the road when we are making more use of the TOSA dialect -- and it will probably be an easy, incremental add at that point.
nit: Keep these sorted.