Page MenuHomePhabricator

TOSA MLIR Dialect
ClosedPublic

Authored by sjarus on Oct 29 2020, 11:27 AM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jsmolens added inline comments.Nov 5 2020, 2:26 PM
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 added inline comments.Nov 5 2020, 3:53 PM
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 added inline comments.Nov 5 2020, 4:33 PM
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 marked 88 inline comments as done.Nov 6 2020, 11:04 AM
sjarus added inline comments.
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 updated this revision to Diff 303501.Nov 6 2020, 11:06 AM
sjarus marked 7 inline comments as done.

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 accepted this revision.Nov 6 2020, 12:13 PM

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 added inline comments.Nov 6 2020, 1:09 PM
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 marked 4 inline comments as done.Nov 6 2020, 1:32 PM
sjarus added inline comments.
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 updated this revision to Diff 303536.Nov 6 2020, 1:33 PM
sjarus marked 3 inline comments as done.

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

sjarus added a comment.Nov 6 2020, 1:34 PM

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

Yes please.

mehdi_amini added inline comments.Nov 6 2020, 5:00 PM
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 updated this revision to Diff 303594.Nov 6 2020, 5:12 PM
sjarus marked an inline comment as done.

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

rriddle added inline comments.Nov 6 2020, 5:33 PM
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
299

Remove trivial braces within this file.

322

nit: Remove the newline here.

328

nit: Remove the newline here.

sjarus marked 32 inline comments as done.Nov 6 2020, 11:22 PM
sjarus added inline comments.
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 updated this revision to Diff 303619.Nov 6 2020, 11:24 PM
sjarus marked 3 inline comments as done.

Updates to feedback from rriddle

sjarus updated this revision to Diff 303620.Nov 6 2020, 11:39 PM

Removed extra path in test/Dialect/Tosa .

rriddle added inline comments.Nov 7 2020, 12:52 AM
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 updated this revision to Diff 303639.Nov 7 2020, 7:39 AM
sjarus marked an inline comment as done.

Removed TosaTraits and TosaTypes headers. Will add when implemented.

stellaraccident accepted this revision.Nov 7 2020, 8:32 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 7 2020, 8:56 AM
Closed by commit rGb28121133d8c: TOSA MLIR Dialect (authored by sjarus, committed by stellaraccident). · Explain Why
This revision was automatically updated to reflect the committed changes.

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 added a comment.Nov 7 2020, 9:10 AM

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.

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.

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.

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)
...

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.

Found the problem and will send a fix shortly.

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