Page MenuHomePhabricator

Add RegionBranchOpInterface to TOSA control flow ops
Needs ReviewPublic

Authored by sjarus on Dec 3 2020, 9:17 AM.

Details

Summary

Follows up on suggested addition deferred in initial TOSA push

Diff Detail

Event Timeline

sjarus created this revision.Dec 3 2020, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 9:17 AM
sjarus requested review of this revision.Dec 3 2020, 9:17 AM

Thanks!

mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
1628

nit: Keep these sorted.

mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
90

Fix the naming style here.

96

nit: You can use BoolAttr here.

114

nit: Add a comment here.

120

Add a comment here and below.

Is there a way to show this in tests?

sjarus marked 5 inline comments as done.Dec 3 2020, 2:35 PM

Incorporated rriddle's feedback and added tests .

sjarus updated this revision to Diff 309370.Dec 3 2020, 2:36 PM

Incorporated feedback.

mehdi_amini added inline comments.Dec 3 2020, 4:11 PM
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)

sjarus added inline comments.Dec 3 2020, 6:19 PM
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.

sjarus updated this revision to Diff 309431.Dec 3 2020, 6:20 PM

Updated tests.

mehdi_amini added inline comments.Dec 3 2020, 7:06 PM
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

sjarus marked 2 inline comments as done.Dec 7 2020, 12:18 PM
sjarus added inline comments.
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.

rriddle added inline comments.Dec 7 2020, 2:29 PM
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.

sjarus added inline comments.Dec 7 2020, 4:30 PM
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 ?

mehdi_amini added inline comments.Dec 7 2020, 4:38 PM
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.

mehdi_amini added inline comments.Dec 7 2020, 4:40 PM
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.

sjarus marked 8 inline comments as done.Dec 9 2020, 4:53 PM

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.

sjarus updated this revision to Diff 310719.Dec 9 2020, 4:54 PM

Bringing this up again. May I proceed with this ?

mehdi_amini added inline comments.Dec 14 2020, 9:25 AM
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
1648

Why this change?

mlir/test/Dialect/Tosa/sccp.mlir
1

Can you remove this option?

42

Can you clarify what the two tests above are doing?

64

Your checks aren't clear to me here?

sjarus added inline comments.Dec 14 2020, 9:38 AM
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.

mehdi_amini added inline comments.Dec 14 2020, 10:50 AM
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:

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.

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!

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.