This is an archive of the discontinued LLVM Phabricator instance.

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
sjarus added inline comments.Nov 5 2020, 7:50 AM
mlir/include/mlir/Dialect/Tosa/IR/TosaInterfaces.td
29

Deleted code.

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

Code cleaned up to use common utility function. Verifier added.

242

The standard tblgen-ed builder worked fine.

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

Done

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

We've updated this, and have added a comment indicating the desire to have a technical discussion on different kinds of broadcasting regimens we have encountered during legalization work, and how they have been addressed here so far.

220

Sorry for the lack of clarity - tests are in development. I simply tried to keep up with all the comments.

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

Added a new test passes implementing coverage of these APIs. The builders are exercised by the ops tests.

sjarus updated this revision to Diff 303123.Nov 5 2020, 7:52 AM
sjarus marked 7 inline comments as done.

Updates contain:

  • Aesthetic cleanups, usage of better MLIR constructs
  • Unit tests
  • WIP rationale document

Thanks. A lot of nits and mechanical updates, and this is likely the last cycle if you fix them.

Do you have commit access, or do you need me to land this for you?

mlir/docs/Rationale/RationaleTOSADialect.md
1 ↗(On Diff #303123)

Looking more at the directory structure, I think this file makes more sense as mlir/docs/Dialects/TOSA.md. Can you just move this there?

(and then I think a top-level heading of "TOSA Dialect Rationale" makes sense as the primary content of the doc right now)

61 ↗(On Diff #303123)

I think we should defer the design discussion to after this patch, but I will have this question at that point:

When lowering the SPLIT op, why can't you RAUW (replace all uses with) of each result with the outputs you calculate here? Routing it through an IDENTITYN essentially does that if I'm reading it right (which I might not be -- it will be helpful to see the whole thing to discuss properly).

75 ↗(On Diff #303123)

I would take this paragraph out and replace with something like:

The COND_IF and WHILE_LOOP operators implement such structured control flow forms and should be lowerable to corresponding ops in the scf dialect. Since the dialect seeks to remain isomorphic with an external, serialized form, the decision was to keep these ops in the dialect (as opposed to deferring completely to scf), and this may be re-evaluated if this turns out to not yield the expected value.

89 ↗(On Diff #303123)

Add: Maintaining the ability to overlap these different representations of quantization parameters (i.e. tensor-carried vs op-carried) is an important capability when considering progressive lowering between uses that expect one scheme vs the other.

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

Always include a description (i.e. assert(myCondition && "desription of constraint")). Here and elsewhere.

159

Remove excess spaces and add description.

mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
958–959

Nit: Would you mind taking a pass through and aligning the style you use for results/outs? Different spacing/line breaks on many. (I wish we had a tablegen formatter)

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

Nit: Remove namespace qualification (here and above in this file).

mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
44–46

Super-nit: I would remove this comment banner and have both .inc include lines together without line breaks between. Possibly lead in with this banner reworded to "Struct and interface includes"

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

Nit: Remove blank lines between namespace statements.

52

Capitalize 'r' ("Real")

53

Nit: End with period.

68

Nit: variables should be camelCased (here and elsewhere in this file)

68

I'd love to see some asserts at the top of this function that enforce sizes of the ArrayRefs and SmallVector.

71

I think you just want SmallVectorImpl<int64_t> here (avoids needing to specialize the size in calls)

72

sp. Initialize (and capitalize)

90

Nit: Remove trivial braces

94

Nit: Remove trivial braces

98

Nit: Remove trivial braces

102

I would invert the sense of the if, break there and then don't have the above nested in the condition.

114

Nit: Remove trivial braces

118

Nit: Remove trivial braces

123

Nit: Remove trivial braces

127

Ditto on inverting the sense and early-breaking.

138

This being a RankedTensorType seems to be an invariant. Change to a cast (here and below) if agree. That will assert whereas like it is now, it will NPE.

160

s/dyn_cast/cast/

162

Nit: This seems like a really large rank to default. Suggest changing to 4.

168

s/dyn_cast/cast/

202

Nit: Remove trivial braces

230

Nit: Remove trivial braces

260

Nit: Remove trivial braces

297–299

I am surprised at needing this. Can you remove and see if anything breaks? (it should be coming from the tablegen side)

302

Nit: remove blank line

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

Remove

81

Nit: Remove trivial braces

212

Ditto: Not sure why this is needed if the tablegen bits were integrated correctly.

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

Nit: Include && "description"

41

Nit: Include && "description" (here and below)

112

&& description

116

&& description

129

nit: should not need mlir namespace

154

Nit: && description

188

Nit: && "description"

195

Nit: mlir namespace not needed

221

Nit: mlir namespace not needed

243

Nit: && "description"

249

Nit: && "description"

mlir/test/Dialect/Tosa/broadcast.mlir
7

// CHECK-NOT: reshape

15

Please change all of these CHECK-NEXT to CHECK.

mlir/test/Dialect/Tosa/ops.mlir
43

Can you take these tf attributes out throughout this test?

mlir/test/Dialect/Tosa/types.mlir
2

I'm not sure what this file is testing. Remove? These are all just built-in types or from other dialects where they are tested.

We'll work on these nits and upload a new patch soon. Since we're new to this, if the next patch looks ok, then please do land it on our behalf as an act of approval.

rriddle requested changes to this revision.Nov 5 2020, 12:12 PM

Took a first pass over.

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

Remove the newline here.

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

Use ifndef here.

71

Please keep this wrapped at 80 characters.

73

Can you do something like: addOperands({input, weigth, bias}) here instead?

87

The else needs to be on the same line as the previous brace.

125

Incorrect formatting here.

130

Same comments here and below. It'd be nice if you could just move these out of line, which would prevent all of these format errors. Most of these builders already look opaque.

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

Can you use tablegen for this dialect definition?

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

This is not necessary, please remove.

379

Please wrap this file at 80 characters.

790

Format is off here.

811

Weird formatting here, same applies to many other cases in this file.

mlir/include/mlir/Dialect/Tosa/Transforms/Passes.h
23

Remove these given that you already include the header file.

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

Remove all uses of mlir::

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

Can you add that now? Otherwise, this can be moved to a later revision.

87

Remove use of llvm:: here, ArrayRef is already re-exported into the MLIR namespace.

115

This looks like a builder that would be autogenerated.

119

Remove unnecessary returns.

143

nit: Just use !=?

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

Prefer to keep namespaces at the top-level, and only use namespaces for classes. This means that the mlir and tosa namespaces should be removed in favor of using namespace <>.

40

Can you use the tablegen pass backend for these instead?

82

Comments should start with a capital and have proper punctuation.

271

Only classes should be in the namespace, please move this to the top-level.

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

Same here.

39

Same comment as the other pass.

This revision now requires changes to proceed.Nov 5 2020, 12:12 PM

There are quite a few formatting errors in the tablegen files, could you take a look at those? It would help if as much of the C++ code was moved to .cpp files so that formatting can be automatic.

mehdi_amini added inline comments.Nov 5 2020, 12:42 PM
mlir/include/mlir/Dialect/Tosa/IR/TosaOpBase.td
159

Clang-format should catch it (and arc runs clang-format for you by the way)

mlir/lib/Dialect/Tosa/Transforms/TosaMakeBroadcastable.cpp
297–299

(I am surprised this builds actually, I thought I added a compiler flag to forbid global registration)

What is missing from the patch is likely the explicit registration plumbing mlir-opt.

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

You should get this from TableGen generated pass declaration

mlir/test/Dialect/Tosa/dynamic_shape.mlir
12

This test does not seem useful at the moment.

mlir/test/Dialect/Tosa/illegal-types.mlir
44 ↗(On Diff #303123)

You're testing the quant dialect and not the TOSA dialect here.
(Actually it seems to apply to everything in this file?)

mlir/test/Dialect/Tosa/inlining.mlir
32

Can you clarify what is the test above doing?
It seems like it would match the input source right now even without any inlining.

70

(Same as above)

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

Note: unless you have custom printer/parser, testing the parsing isn't very useful (same for ops.mlir)

If you have custom verifier though. an invalid.mlir test that would exercise all of your verifier error (but not ones from other dialects)

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
71

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
2

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
12

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
2

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
12

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
71

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
2

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
12

renamed to constrained_shapes.mlir

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

Test removed.

mlir/test/Dialect/Tosa/types.mlir
2

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

2

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
3

Can we remove this file? It looks empty.

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

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
19

Same comment about includes.

52

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
3

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
3

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