This relates to https://github.com/google/iree/issues/8855 .

On a first pass, this looks ok to me. TOSA itself expresses quantization in-op rather than in-tensor and I don't think we've used these ops other than as a cleanup task (e.g. quant.stats, which I had written an identical removal pattern for internally).

In D120204#3334305, @mehdi_amini wrote:

In D120204#3334243, @stellaraccident wrote:

In lieu of looking, I took Feng at his word that this would be ok. I suspect that there are some utility functions that should just exist in TFLite and I think the stats op was used for something that should probably just be in the tfl dialect, dice it is part of their algorithm. I think the TFLite team should take a pass through and fix these items. Happy to delay the patch of it makes that an orderly exit. It did sound like on the original thread, Feng thought this was an ok way to go.

Thanks, I'll look into looping the TFLite team in on this (Feng moved on from TFLite).

Can you check about https://github.com/tensorflow/tensorflow/commit/2c064eb226af66a0d2086eccc6ba3d990a3b5d0f ; this is from your team and you approved this patch initially :)

In D113322#3112990, @rsuderman wrote:

Okay, I will review and integrate this on Monday to guarantee there are not any cascading failures. Looks really great though. I was just starting work on something where this is very useful.

This change keeps the new pad_const field an optional construct and adds a builder to handle it.

Looks good to me! Sorry for the delay, as I was on vacation yesterday.

Undoes the temporary change in https://reviews.llvm.org/rG05cadc6f71555319882ccabf631d2e6410e3fea4 and moves to 3D tensors now that consuming legalizations have been updated.

Thanks for the explanation!

In D103854#2806565, @rsuderman wrote:

Looks good, I am just checking why the windows build failed (and whether its unrelated).

Temporarily support 2D and 3D tensor types as valid.
Legalizations and tests across multiple repositories must be updated to emit 3D type.
Change to supporting just Tosa_Tensor3D once everything is updated.

In D103854#2806111, @rsuderman wrote:

Something appears to be wrong with your patch as its based on a revision not in the LLVM tree. Could you verify your diff history or resubmit based on head?

I have this same thing locally and was planning to push this out, so I'm not going to stop you here :-)

In D102958#2776457, @stellaraccident wrote:

Ah, you were using the web interface. The docs have extra flags that must be included for full context: https://llvm.org/docs/Phabricator.html#id5

I always use arc diff.

Ok now it seems to look a lot better ? If this is alright, please land it on my behalf - it doesn't seem I can do so myself.

Trying using arc diff --update from commandline.

In D102958#2776308, @stellaraccident wrote:

In D102958#2776297, @mehdi_amini wrote:

Can you upload patches with full context please? Right now I can't see which ops are modified here.

I had a similar question but was able to infer based on eliminating the entire classes of restrictions that had been lifted from the spec.

I don't know phab well enough to know why it can't show context (or how to recommend uploading differently) -- Mehdi, do you know?

Thank you once again for these changes!

Thank you for adding this!

In D101958#2741101, @rsuderman wrote:

The implementation looks good however the description needs work. Be more specific than a TOSA version and preface with [mlir][tosa]. It helps contributors to note if this is a CL they should be interested with. Description should be along the lines of "Added tosa.div op and remove tosa.placeholder". For the LLVM repo the summary should focus on what is specifically changing.

This seems to look fine functionally, modulo the inline aesthetic comments.

In D99628#2660479, @stellaraccident wrote:

Do you need these changes landed for you?

Thanks for the review! I'm not sure how to land this myself, or if I can at all.

At this point the best option appear to be to defer this change. Let's revisit this when the SCF dialect has corresponding tests for this interface. Right now, we aren't able to effectively exercise it, and lack access to precedent material on this interface that we can effectively utilize to construct tests, and don't have familiarity with this domain of MLIR. Thank you for your feedback!

Bringing this up again. May I proceed with this ?

I've attempted to reconstruct the tests with insights from the sccp tests. While doing so, I restricted this interface to cond_if() alone to simplify this effort. I've also changed the conditional of the op to just a boolean I1 rather than an unnecessary I1Tensor, allowing the check in getSuccessorRegions() to work without modification.

Updated tests.

Incorporated feedback.

Incorporated rriddle's feedback and added tests .

Thank you for these cleanup additions!

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

Removed TosaTraits and TosaTypes headers. Will add when implemented.

Removed extra path in test/Dialect/Tosa .

Updates to feedback from rriddle

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

In D90411#2379733, @stellaraccident wrote:

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

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

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

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.

Updates contain:

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

Missed one file update in prior commit

Updated to incorporate comments from mehdi_amini and stellaraccident.

In D90411#2363567, @mehdi_amini wrote:

Sure, but can you point out exactly where is it tested in this patch? My basic mental model is: if I change the behavior of any line of code, will it fails when running ninja check-mlir?
Again: the benefit of splitting this is that we can land patches more incrementally, and individual important pieces, like moveOutOfLoop and the LoopLikeOpInterface can be tested in isolation with the patch that land them.

These are both good questions. It seems they could both be resolved if we add tests to exercise these in mlir/test/Dialect/Tosa . If so, could we please do that as a follow on effort ?

I'm happy to review the code right now, but I'd also like the tests to get in with the code and not as a follow-up. In general we don't land patches without tests as far as I know.

In the context of this patch, I just randomly sampled some functions involved here and couldn't find an obvious caller? For example buildQTypeAttrFromMinMax or all the computeMultiplierAndShift*

Updates to incorporate feedback from bondhugula and mehdi_amini .

• Comment formatting
• camelCase vars
• Removed extraneous computeMultiplierAndShift* functions

In D90411#2363111, @mehdi_amini wrote:

Thanks, really nice!
Can you split out some pieces so we can focus on the dialect itself mostly?
I'm thinking about the QuantUtils parts (which aren't used/tested in this patch anyway) and a few things like the TosaMakeBroadcastable pass, or the TosaOpInterface or the moveOutOfLoop() method and similarly things which aren't clearly needed right now.