Page MenuHomePhabricator

[MLIR][TOSA] First lowerings from Tosa to Linalg

Authored by rsuderman on Thu, Jan 7, 10:58 AM.



Initial commit to add support for lowering from TOSA to Linalg. The focus is on
the essential infrastructure for these lowerings and integration with existing

Includes lowerings for a subset of operations including:

abs, add, sub, pow, and, or, xor, left shift, right shift, tanh

Lit tests are used to validate correctness.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Fri, Jan 8, 1:29 PM
rriddle added inline comments.Fri, Jan 8, 1:35 PM
39 ↗(On Diff #315502)

Remove user names from TODO comments.

46–50 ↗(On Diff #315502)
69 ↗(On Diff #315502)

nit: Only prefer auto if it makes the code more readable. Many of this uses could just use the proper type instead.

100 ↗(On Diff #315502)

nit: Can you move this functor out-of-line into a variable before the if? Makes the control flow here pretty hard to read.

149 ↗(On Diff #315502)

Can you just replace the use of operation here with op directly?

175 ↗(On Diff #315502)

Is the to_vector actually necessary? Does the builder of ScalarOp not take a ValueRange?

silvas added inline comments.Fri, Jan 8, 1:54 PM
29 ↗(On Diff #315502)

use getParallelIteratorTypeName() instead of kParallelIterType

60 ↗(On Diff #315502)

Just merge SimplePointwiseConverter into this pattern.

69 ↗(On Diff #315502)

please restrict this to the static shape case for now.

94 ↗(On Diff #315502)

hasRank and getRank != nloops is redundant

96 ↗(On Diff #315502)

do we actually need ComplexType? I don't see any test covering it.

102 ↗(On Diff #315502)

move the dyn_cast into checkValidShapedType

2 ↗(On Diff #315502)

This testing seems really redundant. I think that things like:

// CHECK: absf
%0 = "tosa.abs"(%arg0) : (tensor<f32>) -> tensor<f32>

is sufficient. (with one test that does the more in-depth check of the full linalg op.

6 ↗(On Diff #315502)

also check the indexing maps.

And check higher-rank cases as well.

318 ↗(On Diff #315502)

test this error in only one place.

rsuderman updated this revision to Diff 315550.Fri, Jan 8, 4:32 PM

Handled remaining comments from Mehdi/Sean

silvas added inline comments.Fri, Jan 8, 4:53 PM
27 ↗(On Diff #315550)

these should be static functions not inside an anonymous namespace

44 ↗(On Diff #315550)

Does this have to be a conversion pattern? It isn't doing a type conversion, so a regular RewritePattern would work and reduce complexity I think.

49 ↗(On Diff #315550)

I don't think there's any value in having this be virtual.

56 ↗(On Diff #315550)

can this be a static helper function outside the class?

Also, you might want to comment that it is just this way due to code the amount of templated code.

83 ↗(On Diff #315550)

you can use cast here instead of dyn_cast.

92 ↗(On Diff #315550)

can either of these checks ever fail? I think all these conditions are guaranteed by the tosa op verifiers. If not, can you add a test case that hits this?

107 ↗(On Diff #315550)

hasStaticShape is already guaranteed by the above code.

48 ↗(On Diff #315550)

check at least the full op name (addf in this case)

also can just put all the tosa.* ops in a single function to reduce boilerplate

rsuderman updated this revision to Diff 315560.Fri, Jan 8, 5:42 PM
rsuderman marked 6 inline comments as done.

Change from OpConversionPattern to RewritePattern

mehdi_amini added inline comments.Fri, Jan 8, 6:00 PM
80 ↗(On Diff #315560)

Nit: if t can be null, you can't call cast or dyn_cast on it right above I believe, so I guess this test can be removed?

sjarus added inline comments.Mon, Jan 11, 10:43 AM

Shouldn't this go in include/mlir/Conversion/ ?

rsuderman updated this revision to Diff 315964.Mon, Jan 11, 4:59 PM
rsuderman marked 2 inline comments as done.

Additional round of comments.

rsuderman updated this revision to Diff 315969.Mon, Jan 11, 5:27 PM
rsuderman marked 23 inline comments as done.

Updates to tosa to linalg testing

46–50 ↗(On Diff #315502)

Removed entirely.

60 ↗(On Diff #315502)

The intention is for multi op versions to instead implement the virtual function directly. I have some example lowerings in the pipeline to do that.

44 ↗(On Diff #315550)

Made a RewritePattern.

49 ↗(On Diff #315550)

The main advantage is the rewrite helper is identical between instantiations and won't be duplicated decreasing binary size.

92 ↗(On Diff #315550)

I am attempting to defensively code these practices given how early TOSA support is. Better to verify before lowering than assume valid lowerings.

rriddle added inline comments.Mon, Jan 11, 5:32 PM

Can changes to this file be reverted now?

33 ↗(On Diff #315969)

Remove the outer namespaces here. Anonymous namespaces should be top-level.

61 ↗(On Diff #315969)

nit: Can you use Type here instead.

166 ↗(On Diff #315969)

Prefer using full name resolution for methods. Only classes should really be in namespaces in .cpp files.

43 ↗(On Diff #315969)

nit: auto -> FuncOp

rsuderman updated this revision to Diff 315970.Mon, Jan 11, 5:37 PM

River Riddle comments

rsuderman marked 4 inline comments as done.Mon, Jan 11, 5:38 PM
silvas added inline comments.Mon, Jan 11, 5:41 PM
60 ↗(On Diff #315502)
rsuderman updated this revision to Diff 316279.Tue, Jan 12, 5:07 PM

Changed to a single function for generating generic op contents.

rsuderman marked an inline comment as done.Tue, Jan 12, 5:08 PM
silvas added inline comments.Tue, Jan 12, 5:17 PM
99 ↗(On Diff #316279)

I think we can hit this with the way we have the code structured now. E.g. an integer add op would currently slip through. Maybe a rewriter.notifyMatchFailure instead?

112 ↗(On Diff #316279)

Can you put this in a static helper? I think it will still be duplicated code-size-wise due to the enclosing template.

119 ↗(On Diff #316279)

You can remove this check and use cast<ShapedType>. The verifier for tosa ops guarantees it.

143 ↗(On Diff #316279)

!st is redundant
!st.hasRank() is also redundant (TOSA ops require it)
!st.getElementType().isSignlessIntOrFloat() is redundant I think (TOSA ops require it)

190 ↗(On Diff #316279)

I think you need to handle the case where createLinalgBodyCalculationForElementwiseOp doesn't succeed. (I think just set a bool in the enclosing function and return rewriter.notifyMatchFailure)

124 ↗(On Diff #316279)

Can you test some cases don't get converted?

  • mismatching ranks (rank broadcasting)
  • dynamic shapes
rsuderman updated this revision to Diff 316501.Wed, Jan 13, 1:21 PM

Removed redundant checks, added more failure test cases.

rsuderman updated this revision to Diff 316502.Wed, Jan 13, 1:22 PM
rsuderman marked 2 inline comments as done.

Added missing static qualifier.

rsuderman marked 4 inline comments as done.Wed, Jan 13, 1:23 PM
rsuderman added inline comments.
112 ↗(On Diff #316279)

I moved the whole thing to a static helper. Should guarantee no replication.

silvas accepted this revision.Wed, Jan 13, 4:03 PM
silvas added inline comments.
136 ↗(On Diff #316502)

nit: remove mention of "buffers" here. initTensors seems like a better name?

148 ↗(On Diff #316502)

nit: auto bodyResultTypes = and remove the declaration above.

166 ↗(On Diff #316502)

This name "error" seems very confusing. I read it as "did encounter an error", so setting it to true here is confusing. Suggest calling it didEncounterError and setting it to false here.

mehdi_amini accepted this revision.Wed, Jan 13, 4:14 PM

LGTM in general.

22 ↗(On Diff #316502)

(FYI: We don't need to specify the SmallVector size anymore.)

31 ↗(On Diff #316502)

This kind of helper make me think that they look like a missing build method to be added on the InitTensorOp instead.

44 ↗(On Diff #316502)

Remove trivial braces, here and elsewhere.

rsuderman marked 6 inline comments as done.Thu, Jan 14, 11:16 AM
rriddle accepted this revision.Thu, Jan 14, 11:16 AM


This revision is now accepted and ready to land.Thu, Jan 14, 11:16 AM
This revision was landed with ongoing or failed builds.Thu, Jan 14, 11:25 AM
This revision was automatically updated to reflect the committed changes.