This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Add TOSA f64 type support for const op
ClosedPublic

Authored by AmosLewis on Mar 5 2023, 3:11 PM.

Details

Summary

[mlir][tosa] Add TOSA f64 type support for const op

Diff Detail

Event Timeline

AmosLewis created this revision.Mar 5 2023, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2023, 3:11 PM
AmosLewis requested review of this revision.Mar 5 2023, 3:11 PM
AmosLewis edited the summary of this revision. (Show Details)Mar 5 2023, 3:12 PM
AmosLewis added a reviewer: jpienaar.

This patch fix torch.prim.NumToTensor.Scalar float64 support https://github.com/llvm/torch-mlir/pull/1802
This fix should have been added in
https://reviews.llvm.org/rGa2dcd994a7f8cc33640f58105276b78acf3483e5

jpienaar added inline comments.Mar 5 2023, 9:05 PM
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
1824

This naming and comment around the def is confusing now. Could you update?

AmosLewis updated this revision to Diff 502827.Mar 6 2023, 3:21 PM
AmosLewis edited the summary of this revision. (Show Details)

Chang the tensor with f64 support name from Tosa_Tensor_Cast to Tosa_Tensor_F64

AmosLewis updated this revision to Diff 502831.Mar 6 2023, 3:28 PM
AmosLewis marked an inline comment as done.

Chang the tensor with f64 support name from Tosa_Tensor_Cast to Tosa_Tensor_64

AmosLewis added inline comments.Mar 6 2023, 3:30 PM
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
1824

How about now? I name all the tensor support f64 with Tosa_Tensor_64.

This is definitely an improvement, I'm okay with this type, but it reads as only a 64-bit tensor. Perhaps Jacques is better at naming than I am, but I'd go something closer to Tosa_Tensor_Plus_F64, indicating that this is an extension of Tosa_Tensor with F64 (as opposed to I64).

AmosLewis updated this revision to Diff 502877.Mar 6 2023, 5:51 PM

Chang the tensor with f64 support name from Tosa_Tensor_Cast to Tosa_Tensor_Plus_F64

This is definitely an improvement, I'm okay with this type, but it reads as only a 64-bit tensor. Perhaps Jacques is better at naming than I am, but I'd go something closer to Tosa_Tensor_Plus_F64, indicating that this is an extension of Tosa_Tensor with F64 (as opposed to I64).

Done

eric-k256 accepted this revision.Mar 7 2023, 11:46 AM

Looks reasonable to me, although I don't have merge priveleges.

This revision is now accepted and ready to land.Mar 7 2023, 11:46 AM
manupak added a subscriber: manupak.EditedMar 8 2023, 3:14 AM

Quick question @eric-k256 @AmosLewis,

Will it also fold it to be non-F64 ? i.e. tosa consumers are able to not to witness f64 when tosa is in the canonical form.

jpienaar accepted this revision.Mar 8 2023, 7:07 AM

Quick question @eric-k256 @AmosLewis,

Will it also fold it to be non-F64 ? i.e. tosa consumers are able to not to witness f64 when tosa is in the canonical form.

I think this should be explicit pass as it may be interprocedural. Consumers post the "verify for profile" (or runtime, forgot what it is called) should not be getting these. Either by way of it errors out during that verify pass or converted to f32 (or some such pass, which would be opt in and may change precision).

For me canonical would be a function of profile.

(which effectively means no not canonical, but one has sections with clear expectations and post "deployment" profile verification there should be none of these for consumers)

manupak added a comment.EditedMar 8 2023, 9:30 AM

Thanks, sounds like a no for now and agree with interprocedural implications.

I was curious because https://reviews.llvm.org/D142599 sounded like F64 wasn't in any profile
and also there are folders for splat based consts (which might help a bit) that also have the same said implications.

You're right that F64 isn't in any of the TOSA profiles today, which is why I was discouraging it's use in the overall definition of Tosa_Tensor. It's something we look at adding, but it adds significant requirements to any profiles it goes into. It would be good to get a sense of what networks need F64 vs ending up with that type as default. Many systems that support f64 do it at a performance cost against f32, and some systems don't implement it at all. Ideally the tooling would have a way to guide developers to minimize their use of f64 to where it has a significant improvement on results justifying the extra computation cost.

I concur.

Hence, the question: should we also look to fold it as much as possible ?

I think the cases we have seen are just F64 constants being created (maybe due to default python behaviour?) and then immediately casted to lower bitwidths.
Exactly what this patch is doing in other words -- so its a step in the right direction.

I think splats are folded; so we are good there - it might cover most bases.

I was just curious whether extending the folding of tosa.cast to tosa.const operands to limit the appearance of f64 where possible -- was a good idea or not, while we are here.
Especially if we can catch them before outlining -- i.e. scenarios where we wouldn't hit the inter-procedural case.

Quick question @eric-k256 @AmosLewis,

Will it also fold it to be non-F64 ? i.e. tosa consumers are able to not to witness f64 when tosa is in the canonical form.

I think this should be explicit pass as it may be interprocedural. Consumers post the "verify for profile" (or runtime, forgot what it is called) should not be getting these. Either by way of it errors out during that verify pass or converted to f32 (or some such pass, which would be opt in and may change precision).

For me canonical would be a function of profile.

@jpienaar could you land this patch?

Can you add tests please?

AmosLewis updated this revision to Diff 504934.Mar 13 2023, 9:09 PM

[mlir][tosa] Add a test for tosa::const 64

AmosLewis updated this revision to Diff 504935.Mar 13 2023, 9:10 PM

delete extra line

Can you add tests please?

Done

@jpienaar could you help land this patch when you are available?

This revision was automatically updated to reflect the committed changes.