This is the TOSA MLIR Dialect described in the following MLIR RFC: https://llvm.discourse.group/t/rfc-tosa-dialect-in-mlir/1971/24
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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. |
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 ? |
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
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). |
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 |
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) |
|
1 ↗ | (On Diff #303123) | Test removed. |
stellaraccident: Removed extraneous tests as suggested
mehdi_amini: added test decorators.
mlir/test/Dialect/Tosa/inlining.mlir | ||
---|---|---|
37 | Should be // CHECK-NOT: @while |
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 | |
299 | Remove trivial braces within this file. | |
322 | nit: Remove the newline here. | |
328 | nit: Remove the newline here. |
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. |
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. |
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.
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.
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.
Fixed in https://reviews.llvm.org/D91022 (just waiting on the bots and then
will submit).