TOSA MLIR Dialect

[sjarus]

Summary

This is the TOSA MLIR Dialect described in the following MLIR RFC: https://llvm.discourse.group/t/rfc-tosa-dialect-in-mlir/1971/24

[jsmolens]

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

958–959

Could you kindly point us to an example of the preferred formatting somewhere else in the source tree and we'll do our best to match it.

It looks like 2 indents, but we're not clear on where other newlines or alignment should happen.

[stellaraccident]

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

958–959

There is not a ton of consistency, afaik. However, I was just referring to consistency within this file. Literally reading from one op to the next, the wrapping and indentation are pretty different. Like I said, though: nit.

[jsmolens]

mlir/test/Dialect/Tosa/quant-ops.mlir

70 ↗

(On Diff #303123)

We feel the op tests do provide some value in testing that the ops definitions in the .td files match up with what should be considered valid ops. That is, we're testing that the ops have been defined correctly.

Hopefully the working parsing code is already a given!

The tests can be removed if such tests haven't proven to be useful in other dialects.

mlir/test/Dialect/Tosa/types.mlir

1 ↗

(On Diff #303123)

We tried to add coverage for the types that are used by TOSA, so perhaps it's the sum of the tests that's valuable rather than the tests on their own. That said, I suspect the types are unlikely to go away anytime soon and we don't feel strongly about retaining them if this is considered a weak justification.

[sjarus]

mlir/docs/Rationale/RationaleTOSADialect.md

61 ↗

(On Diff #303123)

Yes we'll have the legalization piece out soon - both pieces work together right now, and ensuring we tested this whole setup took extra time.

mlir/include/mlir/Dialect/Tosa/IR/TosaOpBase.td

130

Moved the builders out of TableGen.

159

clang-format is part of our flow but it apparently had trouble within tblgen files. However these builders were moved into C++ incorporating rriddle's feedback, so it now formats properly.

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

115

Removed, using autogenerated builder

mlir/test/Dialect/Tosa/dynamic_shape.mlir

11 ↗

(On Diff #303123)

When we added constrained shape types (e.g. Tosa_TensorUpto4D, we wanted to check what happens when a tensor with unspecified shape is passed - would that mechanism fail ? This test was to validate that it was ok. I guess the MLIR folks already knew the answer but we did not, and so this test happened. We see it as helpful just to validate that any new constraints don't break anything from this perspective.

mlir/test/Dialect/Tosa/illegal-types.mlir

44 ↗

(On Diff #303123)

File removed.

mlir/test/Dialect/Tosa/inlining.mlir

32

They were created for the dialect and while body inliner checking. Updated both tests to fix some minor issues. The first one emits the following inlined form:

module {
  func @inlined_if_fn(%arg0: tensor<f32>, %arg1: tensor<f32>, %arg2: tensor<i1>) -> tensor<f32> {
    %0 = "tosa.cond_if"(%arg2, %arg0, %arg1) ( {
    ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):  // no predecessors
      %1 = "tosa.add"(%arg3, %arg4) : (tensor<f32>, tensor<f32>) -> tensor<f32>
      "tosa.yield"(%1) : (tensor<f32>) -> ()
    },  {
    ^bb0(%arg3: tensor<f32>, %arg4: tensor<f32>):  // no predecessors
      %1 = "tosa.sub"(%arg3, %arg4) : (tensor<f32>, tensor<f32>) -> tensor<f32>
      "tosa.yield"(%1) : (tensor<f32>) -> ()
    }) : (tensor<i1>, tensor<f32>, tensor<f32>) -> tensor<f32>
    return %0 : tensor<f32>
  }
}

The second one generates this:

module {
  func @inlined_while_fn(%arg0: tensor<i32>, %arg1: tensor<i32>, %arg2: tensor<i32>, %arg3: tensor<10xi32>) -> tensor<10xi32> {
    %0:4 = "tosa.while_loop"(%arg0, %arg1, %arg2, %arg3) ( {
    ^bb0(%arg4: tensor<i32>, %arg5: tensor<i32>, %arg6: tensor<i32>, %arg7: tensor<10xi32>):  // no predecessors
      %1 = "tosa.greater_equal"(%arg4, %arg5) : (tensor<i32>, tensor<i32>) -> tensor<i1>
      %2 = "tosa.logical_not"(%1) : (tensor<i1>) -> tensor<i1>
      "tosa.yield"(%2) : (tensor<i1>) -> ()
    },  {
    ^bb0(%arg4: tensor<i32>, %arg5: tensor<i32>, %arg6: tensor<i32>, %arg7: tensor<10xi32>):  // no predecessors
      %1 = "tosa.add"(%arg4, %arg5) : (tensor<i32>, tensor<i32>) -> tensor<i32>
      %2 = "tosa.add"(%arg7, %1) : (tensor<10xi32>, tensor<i32>) -> tensor<10xi32>
      "tosa.yield"(%1, %arg5, %arg6, %2) : (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) -> ()
    }) : (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>) -> (tensor<i32>, tensor<i32>, tensor<i32>, tensor<10xi32>)
    return %0#3 : tensor<10xi32>
  }
}

However, it's unclear how to use the test decorators properly in this case and we'd appreciate any help with adding them properly, but I can confirm that they do inline the input in this manner.

mlir/test/Dialect/Tosa/types.mlir

1 ↗

(On Diff #303123)

We created tests for every type that Tosa uses. Is that going too far ?

[sjarus]

Incorporates feedback from stellaraccident, rriddle, mehdi_amini

• Cleanup nits and mechanicals
• Removed extraneous tests
• Quantization Builders in C++ now
• Aligned pass invocation with current approach
• 80-col alignment

[stellaraccident]

I believe we are at or fairly close to completion on this revision. Thank you for iterating on all of the things that come easier with experience in the project.

I assume you will need me/someone to land this change? (and if not, give some time for other reviewers to comment further)

mlir/test/Dialect/Tosa/dynamic_shape.mlir

11 ↗

(On Diff #303123)

I'm fine keeping the test if it is verifying a corner case of the predicate that you want to ensure stays valid. However, I would probably rename it to account for what it is actually trying to verify and note that you are just using the argmax op as a canonical example of an op that exercises it (i.e. @test_tensor4d_constraint_allows_dynamic_shape).

mlir/test/Dialect/Tosa/quant-ops.mlir

70 ↗

(On Diff #303123)

Given the relatively type constraints and verifiers, especially around quantized types, keeping some set of these makes sense to me, but do note Mehdi's point about testing the negative verification cases in an invalid.mlir file.

mlir/test/Dialect/Tosa/types.mlir

1 ↗

(On Diff #303123)

Your reasoning makes sense as a standalone project, but within MLIR itself, we consider the sum total of tests across the project to be the key thing. In this case, it is best if dialect specific tests cover the types/attributes that they introduce (custom types/attributes) but don't have explicit tests for types from the rest of the project (unless if there is a non trivial interplay, which there does not seem to be here).

[mehdi_amini]

mlir/include/mlir/Dialect/Tosa/IR/TosaOpBase.td

159

I missed that it was a .td file, makes sense!

mlir/test/Dialect/Tosa/inlining.mlir

32

I would likely do something like this for the first one :

// CHECK-LABEL: func @inlined_if_fn
// Check that both the calls and the functions are eliminated after inlining:
// CHECK-NOT: @add
// CHECK-NOT: @sub

And similarly for the second one:

// Check that calls are inlined and functions eliminated:
// CHECK-NOT: @while

[sjarus]

mlir/test/Dialect/Tosa/dynamic_shape.mlir

11 ↗

(On Diff #303123)

renamed to constrained_shapes.mlir

mlir/test/Dialect/Tosa/quant-ops.mlir

70 ↗

(On Diff #303123)

Test removed.

mlir/test/Dialect/Tosa/types.mlir

1 ↗

(On Diff #303123)

Your reasoning makes sense as a standalone project, but within MLIR itself, we consider the sum total of tests across the project to be the key thing. In this case, it is best if dialect specific tests cover the types/attributes that they introduce (custom types/attributes) but don't have explicit tests for types from the rest of the project (unless if there is a non trivial interplay, which there does not seem to be here).

1 ↗

(On Diff #303123)

Test removed.

[sjarus]

stellaraccident: Removed extraneous tests as suggested
mehdi_amini: added test decorators.

[sjarus]

In D90411#2379733, @stellaraccident wrote:

I assume you will need me/someone to land this change?

Yes please.

[mehdi_amini]

mlir/test/Dialect/Tosa/inlining.mlir

37

Should be // CHECK-NOT: @while
Right now I believe the test would still pass if we don't inline anything

[sjarus]

Fixed typo in inlining test. Reran check-mlir to confirm.

[rriddle]

mlir/docs/Dialects/TOSA.md

2

Can you apply a markdown formatter to this file?

mlir/include/mlir/Dialect/Tosa/IR/TosaInterfaces.td

25

This is the default, so this line can be removed.

mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h

24

Can you trim some of these? Several look unnecessary(e.g. Attributes/OpDefinition/StandardTypes)

44

MLIR functions should use camelCase.

44

Do any of these files need to be in the header? These are generally implemented only in the .cpp file. If they do need to be shared by multiple source files, these can be moved to a private header in the lib directory.

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

51

Please keep all includes at the top of the file.

111

The summary should be kept to "" string blocks. The same applies to several ops in this file.

1704

Can we mark this operation with RecursiveSideEffects? Same applies to the other region control flow operations.

1704

Do you intend to add the RegionBranchOpInterface interface to these control flow operations? (Would give things like constant propagation/etc. for free).

1735

Can we tag with NoSideEffect here?

mlir/include/mlir/Dialect/Tosa/IR/TosaTraits.h

2 ↗

(On Diff #303536)

Can we remove this file? It looks empty.

mlir/include/mlir/Dialect/Tosa/IR/TosaTypes.h

2 ↗

(On Diff #303536)

Can we remove this file? It looks empty.

mlir/include/mlir/Dialect/Tosa/IR/TosaTypesBase.td

81

nit: Can you indent the F32 the same as the rest?

mlir/include/mlir/Dialect/Tosa/Transforms/Passes.td

31

Can you move the test passes to the test/lib/ directory instead? We try to keep the main tree clean from these.

mlir/include/mlir/Dialect/Tosa/Utils/QuantUtils.h

22

nit: Move these above the comment block just above.

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

20

Can you trim some of these? Many look unnecessary (e.g. Attributes/Diagnostics/MLIRContext/Types/LLVM/LogicalResult/etc)

113

nit: Remove the newline here.

144

nit: Please use /// for function level comments, here and below.

163

Please use braces for all parts of the if, if any other part has braces.

183

Same here and below.

mlir/lib/Dialect/Tosa/Transforms/TosaMakeBroadcastable.cpp

19

Same comment here about trimming includes.

119

Missing static here?

144

Use // for comments.

224

Classes need to be placed within an anonymous namespaces (functions should be marked static and top level).

250

nit: Any reason not to just inline this into the runOnFunction pass below?

274

Is there any part of this pass that requires FuncOp? Can you make this a generic operation pass instead? That would increase potential reuse.

mlir/lib/Dialect/Tosa/Transforms/TosaTestPasses.cpp

18 ↗

(On Diff #303536)

Same comment about includes.

51 ↗

(On Diff #303536)

nit: Can you move these function definitions out of the namespace? The namespace should be kept as small as possible.

mlir/lib/Dialect/Tosa/Utils/QuantUtils.cpp

17

Only uses namespaces for classes, and prefer using explicit qualification as opposed to namespaces in .cpp files.

https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

299

Remove trivial braces within this file.

322

nit: Remove the newline here.

328

nit: Remove the newline here.

[sjarus]

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

51

It was there because Tosa_Dialect is accessed by the attributes in TosaOpBase.td. I just moved the dialect definition into TosaOpBase.td to resolve this right.

1704

Deferring RegionBranchOpInterface until later. We would like to try it out first since it is new to us.

mlir/include/mlir/Dialect/Tosa/IR/TosaTraits.h

2 ↗

(On Diff #303536)

We intend to develop traits as part of further work so we left is file here. Same for TosaTypes.

[sjarus]

Updates to feedback from rriddle

[sjarus]

Removed extra path in test/Dialect/Tosa .

[rriddle]

mlir/include/mlir/Dialect/Tosa/IR/TosaTraits.h

2 ↗

(On Diff #303536)

Can you please remove these files until you add the corresponding Types/Traits then? We generally prefer not to add files/code/etc. that are unused. It's good practice to add them when they are needed.

[sjarus]

Removed TosaTraits and TosaTypes headers. Will add when implemented.

[stellaraccident]

Thank you for the diligent reviews, everyone. I took a final pass through and believe that this patch has converged sufficient to land. I also landed an NFC followon to address some items I found when actually going through it in my editor and building (https://reviews.llvm.org/rGb5fcd06105dec2a7b0e4114d6ad4524fc54498c5).

Suraj: Thank you for the patience, and I believe that the epic 204 comments signifies why Mehdi advised at the outset that breaking the patch up may be advantageous (to be kept in mind for the future), especially as a first commit that is still dialing in style and conventions.

[sjarus]

To suit the flavor of the season, we need a recount here - there was more than 204 since some appear to have disappeared along with deleted files :-)

We'd like to thank you all for your patience in kind. On our part as well, we understand it's not easy to make such a large contribution into such a critical infrastructure, and we did everything we could to help get this through.

[rriddle]

In D90411#2380871, @stellaraccident wrote:

Thank you for the diligent reviews, everyone. I took a final pass through and believe that this patch has converged sufficient to land. I also landed an NFC followon to address some items I found when actually going through it in my editor and building (https://reviews.llvm.org/rGb5fcd06105dec2a7b0e4114d6ad4524fc54498c5).

Suraj: Thank you for the patience, and I believe that the epic 204 comments signifies why Mehdi advised at the outset that breaking the patch up may be advantageous (to be kept in mind for the future), especially as a first commit that is still dialing in style and conventions.

In the future can you please refrain from submitting before reviewers have approved, I wasn't completely done with my final pass.

[stellaraccident]

Will do. The review thread was getting confusing. If you have further comments, I will work with you to land them in a follow-up.

[nathanchance]

I have not done a bisect or anything but I suspect this commit broke -DLLVM_INCLUDE_TESTS=OFF:

$ cmake \
-G Ninja \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_COMPILER=clang \
-DCMAKE_CXX_COMPILER=clang++ \
-DLLVM_CCACHE_BUILD=ON \
-DLLVM_ENABLE_PROJECTS=mlir \
-DLLVM_ENABLE_WARNINGS=OFF \
-DLLVM_INCLUDE_TESTS=OFF \
-DLLVM_TARGETS_TO_BUILD=X86 \
-DLLVM_USE_LINKER=lld \
../llvm && \
ninja
...
[2535/2573] Linking CXX executable bin/mlir-opt
FAILED: bin/mlir-opt
: && /home/nathan/usr/bin/clang++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -w -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fuse-ld=lld -Wl,--color-diagnostics    -Wl,-O3 -Wl,--gc-sections tools/mlir/tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o -o bin/mlir-opt  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMCore.a  lib/libLLVMSupport.a  lib/libLLVMAsmParser.a  -lpthread  lib/libMLIRAffine.a  lib/libMLIRAffineEDSC.a  lib/libMLIRAffineTransforms.a  lib/libMLIRAffineUtils.a  lib/libMLIRAsync.a  lib/libMLIRAVX512.a  lib/libMLIRGPU.a  lib/libMLIRLinalgAnalysis.a  lib/libMLIRLinalgEDSC.a  lib/libMLIRLinalg.a  lib/libMLIRLinalgTransforms.a  lib/libMLIRLinalgUtils.a  lib/libMLIRLLVMIRTransforms.a  lib/libMLIRLLVMIR.a  lib/libMLIRLLVMAVX512.a  lib/libMLIRNVVMIR.a  lib/libMLIRROCDLIR.a  lib/libMLIROpenACC.a  lib/libMLIROpenMP.a  lib/libMLIRPDL.a  lib/libMLIRPDLInterp.a  lib/libMLIRQuant.a  lib/libMLIRSCF.a  lib/libMLIRSCFTransforms.a  lib/libMLIRSDBM.a  lib/libMLIRShape.a  lib/libMLIRShapeOpsTransforms.a  lib/libMLIRSPIRV.a  lib/libMLIRSPIRVModuleCombiner.a  lib/libMLIRSPIRVSerialization.a  lib/libMLIRSPIRVTransforms.a  lib/libMLIRStandard.a  lib/libMLIRStandardOpsTransforms.a  lib/libMLIRTosa.a  lib/libMLIRTosaTransforms.a  lib/libMLIRVector.a  lib/libMLIRAffineToStandard.a  lib/libMLIRAsyncToLLVM.a  lib/libMLIRAVX512ToLLVM.a  lib/libMLIRGPUToGPURuntimeTransforms.a  lib/libMLIRGPUToNVVMTransforms.a  lib/libMLIRGPUToROCDLTransforms.a  lib/libMLIRGPUToSPIRVTransforms.a  lib/libMLIRGPUToVulkanTransforms.a  lib/libMLIRLinalgToLLVM.a  lib/libMLIRLinalgToSPIRVTransforms.a  lib/libMLIRLinalgToStandard.a  lib/libMLIROpenMPToLLVM.a  lib/libMLIRPDLToPDLInterp.a  lib/libMLIRSCFToGPU.a  lib/libMLIRSCFToSPIRV.a  lib/libMLIRSCFToStandard.a  lib/libMLIRShapeToStandard.a  lib/libMLIRSPIRVToLLVM.a  lib/libMLIRStandardToLLVM.a  lib/libMLIRStandardToSPIRVTransforms.a  lib/libMLIRVectorToROCDL.a  lib/libMLIRVectorToLLVM.a  lib/libMLIRVectorToSCF.a  lib/libMLIRVectorToSPIRV.a  lib/libMLIRLoopAnalysis.a  lib/libMLIRAnalysis.a  lib/libMLIRDialect.a  lib/libMLIREDSC.a  lib/libMLIROptLib.a  lib/libMLIRParser.a  lib/libMLIRPass.a  lib/libMLIRTransforms.a  lib/libMLIRTransformUtils.a  lib/libMLIRSupport.a  lib/libMLIRIR.a  lib/libMLIRLinalgAnalysis.a  lib/libMLIRTosa.a  lib/libMLIRQuant.a  lib/libMLIRAsync.a  lib/libMLIRAVX512.a  lib/libMLIRLLVMAVX512.a  lib/libMLIRNVVMIR.a  lib/libMLIRROCDLIR.a  lib/libMLIRSPIRVSerialization.a  lib/libMLIRVectorToLLVM.a  lib/libMLIRTargetLLVMIRModuleTranslation.a  lib/libMLIRLLVMIRTransforms.a  lib/libMLIRTranslation.a  lib/libLLVMFrontendOpenMP.a  lib/libLLVMTransformUtils.a  lib/libMLIRLinalgUtils.a  lib/libMLIRLinalgEDSC.a  lib/libMLIROpenMP.a  lib/libMLIRPDLInterp.a  lib/libMLIRPDL.a  lib/libMLIRAffineToStandard.a  lib/libMLIRShape.a  lib/libMLIRDialect.a  lib/libMLIRGPU.a  lib/libMLIRStandardToLLVM.a  lib/libMLIRLLVMIR.a  lib/libLLVMAsmParser.a  lib/libLLVMBitWriter.a  lib/libLLVMAnalysis.a  lib/libLLVMProfileData.a  lib/libLLVMObject.a  lib/libLLVMBitReader.a  lib/libLLVMCore.a  lib/libLLVMRemarks.a  lib/libLLVMBitstreamReader.a  lib/libLLVMMCParser.a  lib/libLLVMMC.a  lib/libLLVMDebugInfoCodeView.a  lib/libLLVMDebugInfoMSF.a  lib/libLLVMTextAPI.a  lib/libLLVMBinaryFormat.a  lib/libMLIRStandardOpsTransforms.a  lib/libMLIRSPIRV.a  lib/libMLIRTransforms.a  lib/libMLIRVector.a  lib/libMLIRAffineEDSC.a  lib/libMLIRLinalg.a  lib/libMLIRTransformUtils.a  lib/libMLIRLoopAnalysis.a  lib/libMLIRPresburger.a  lib/libMLIRRewrite.a  lib/libMLIRCopyOpInterface.a  lib/libMLIRParser.a  lib/libMLIRPass.a  lib/libMLIRAnalysis.a  lib/libMLIRAffine.a  lib/libMLIRSCF.a  lib/libMLIRStandard.a  lib/libMLIRViewLikeInterface.a  lib/libMLIRVectorInterfaces.a  lib/libMLIREDSC.a  lib/libMLIRLoopLikeInterface.a  lib/libMLIRSideEffectInterfaces.a  lib/libMLIRCallInterfaces.a  lib/libMLIRControlFlowInterfaces.a  lib/libMLIRInferTypeOpInterface.a  lib/libMLIRIR.a  lib/libMLIRSupport.a  lib/libLLVMSupport.a  -lrt  -ldl  -lm  /usr/lib/x86_64-linux-gnu/libz.so  /usr/lib/x86_64-linux-gnu/libtinfo.so  lib/libLLVMDemangle.a  -lpthread && :
ld.lld: error: undefined symbol: mlir::tosa::createTosaTestQuantUtilAPIPass()
>>> referenced by mlir-opt.cpp
>>>               tools/mlir/tools/mlir-opt/CMakeFiles/mlir-opt.dir/mlir-opt.cpp.o:(std::_Function_handler<std::unique_ptr<mlir::Pass, std::default_delete<mlir::Pass> > (), mlir::tosa::registerTosaTestQuantUtilsPass()::'lambda'()>::_M_invoke(std::_Any_data const&))
clang-12: error: linker command failed with exit code 1 (use -v to see invocation)
...

[stellaraccident]

Thank you for the report: I don't think we have a bit which verifies that,
so it is plausible. I will triage this momentarily.

[stellaraccident]

Found the problem and will send a fix shortly.

[stellaraccident]

Fixed in https://reviews.llvm.org/D91022 (just waiting on the bots and then
will submit).