This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Delete most of the quant dialect.
ClosedPublic

Authored by stellaraccident on Feb 19 2022, 10:08 PM.

Details

Summary
  • https://discourse.llvm.org/t/rfc-removing-the-quant-dialect/3643/8
  • Removes most ops. Leaves casts given final comment (can remove more in a followup).
  • There are a few uses in Tosa keeping some of the utilities alive. In a followup, I will probably elect to just move simplified versions of them into Tosa itself vs having this quasi-library dependency.
  • I will plan to submit this in 2 weeks: I suspect that some of the items deleted are in use in downstreams (and really shouldn't be) and will give time to adapt.
  • Please speak up if you need more time.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Feb 19 2022, 10:08 PM
mehdi_amini added inline comments.Feb 20 2022, 10:31 AM
mlir/include/mlir/Dialect/Quant/QuantOps.td
202

I still see a bunch of uses for this op in TF-to-TOSA and TFLite right now. Do you know more about this?

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.

mehdi_amini added a comment.EditedFeb 20 2022, 1:01 PM

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

Yes, that patch was my first indication that things here may not have been as assumed. I think that if the stats op is used by tflite's algorithms, it should be part of the tfl dialect. If that were the case, the above patch would be a fairly trivial fix that just drops these unneeded metadata ops. Happy to fix that if the root dependence on the op is fixed, but I suspect it will be easier for whoever does it to just rename quant->tfl for this case as part of the bigger refactor.

sjarus added a subscriber: sjarus.EditedFeb 20 2022, 7:47 PM

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

TFLite change in the TensorFlow repo LGTM to me too. We have this same pattern in a separate internal cleanup pass . We'll remove it there now that it's here.

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

I assume there's no plan to eliminate the definition of MLIR quantized types (not ops) in a separate diff ?

Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 5:17 PM
Herald added a subscriber: bzcheeseman. · View Herald Transcript
jpienaar accepted this revision.Jul 27 2022, 5:38 PM
jpienaar added a subscriber: jpienaar.

Thanks for heads-up and following up this.

This revision is now accepted and ready to land.Jul 27 2022, 5:38 PM
This revision was landed with ongoing or failed builds.Jul 27 2022, 5:51 PM
This revision was automatically updated to reflect the committed changes.
mlir/unittests/Dialect/CMakeLists.txt