This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Constant optimizations for reduce operations
ClosedPublic

Authored by amirBish on Jul 10 2023, 4:50 AM.

Details

Summary

Replace the different reduce operations which is getting
a constant tensor as an input argument with a constant
tensor.

As the arguement of the reduce operation is constant tensor
and has only a single user we could calculate the resulted
constant tensor in compilation time and replace it
with reduced memory tensor

This optimization has been implemented for:
tosa.reduce_sum
tosa.reduce_prod
tosa.reduce_any
tosa.reduce_all
tosa.reduce_max
tosa.reduce_min

Diff Detail

Event Timeline

amirBish created this revision.Jul 10 2023, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 4:50 AM
amirBish requested review of this revision.Jul 10 2023, 4:50 AM
rsuderman requested changes to this revision.Jul 10 2023, 10:39 AM
rsuderman added inline comments.
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
1412

Mind breaking this into a couple lines? clang-format does not understand tablegen sadly.

1444

Ditto.

mlir/lib/Dialect/Tosa/Transforms/TosaReduceConstantOptimization.cpp
37 ↗(On Diff #538581)

You are in an empty namespace so you don't need static. Also, declare this outside of the struct.

63 ↗(On Diff #538581)

Ditto.

64 ↗(On Diff #538581)

Can you use a more simple type for this? This feels likely simpler as a regular attribute.

67 ↗(On Diff #538581)

You should be using llvm::SmallVector and llvm::ArrayRef instead of std::vector and std::vector&

68 ↗(On Diff #538581)

You only return a single thing so its better to compute and return than take a std::vector&.

75 ↗(On Diff #538581)

You do not need to recompute the position every time. Just determine what the striding is and do position = initial + index * stride.

77 ↗(On Diff #538581)

Initialize the value before the loop and start iterating at 0. This avoids doing a index == 0 check every iteration.

90 ↗(On Diff #538581)

You will also want to check the resultTy and verify the return type is known and statically shaped. Otherwise you could cause some type changes for under-defined programs.

111 ↗(On Diff #538581)

This should be a llvm::SmallVector

113 ↗(On Diff #538581)

you can request the new shape from the result type. Just invoke getType() and case to a shape type.

117 ↗(On Diff #538581)

Same about llvm::SmallVector<APInt>

119 ↗(On Diff #538581)

Name is extremely verbose. See if you can reduce it to something simpler. E.g. reductionIndex.

mlir/test/Dialect/Tosa/reduce-constant-optimizations.mlir
1 ↗(On Diff #538581)

Delete extra startine line.

1 ↗(On Diff #538581)

Please merge this file with the other tosa constant folding tests. We should not have 3 different files for constant folding.

481 ↗(On Diff #538581)

Missing newline at end.

This revision now requires changes to proceed.Jul 10 2023, 10:39 AM
amirBish updated this revision to Diff 538958.EditedJul 11 2023, 1:22 AM

updating the commit based on rsuderman reply

amirBish marked 14 inline comments as done.Jul 11 2023, 1:34 AM
amirBish added inline comments.
mlir/lib/Dialect/Tosa/Transforms/TosaReduceConstantOptimization.cpp
64 ↗(On Diff #538581)

sure, passed the mlir::elementsAttr instead of the complex type

67 ↗(On Diff #538581)

I've modified all the code to use SmallVector/ArrayRef. also used the MutableArrayRef for non const reference, thanks

68 ↗(On Diff #538581)

modified the function to return the llvm::APInt as a reducedValue, and there is no need to take the std::vector&

90 ↗(On Diff #538581)

thanks, I've missed it

119 ↗(On Diff #538581)

modified to reductionIndex

amirBish marked 5 inline comments as done.Jul 11 2023, 1:35 AM

@rsuderman would appreciate your feedback regarding the recent changes

Sorry for the delay

mlir/lib/Dialect/Tosa/Transforms/TosaReduceConstantOptimization.cpp
64 ↗(On Diff #538958)

llvm::MutableArrayRef only makes sense when you are intending to inplace mutate the ArrayRef for a return. See the change below, but you should only need position to be a single integer.

72 ↗(On Diff #538958)

Make the uint64_t match the element type of oldShape instead of casting the value of oldShape[reductionAxis]

79 ↗(On Diff #538958)

Prefer a new value to mutation. It is better in this case to do

index = indexAtOldTensor += stride * reductionAxisVal;

and use index instead.

130 ↗(On Diff #538958)

You should be this work into the calculateReduceValue as it is only used in there. You also do not need the newShape if you have the oldShape and reductionAxis, so only need to pass the reductionIndex in instead of the position.

137 ↗(On Diff #538958)

Why not just use op.getType().cast<RankedTensorType>() instead of constructing a new one?

mlir/test/Dialect/Tosa/constant-op-fold.mlir
577

I do not see a single test for floating-point numbers. Have you tested what its behavior is with floating point?

amirBish updated this revision to Diff 543224.Jul 22 2023, 1:17 PM

Additional changes according to rsuderman's review

amirBish marked 5 inline comments as done.Jul 22 2023, 1:38 PM

thanks for the reply @rsuderman , uploaded a new revision

mlir/lib/Dialect/Tosa/Transforms/TosaReduceConstantOptimization.cpp
64 ↗(On Diff #538958)

you're right, It's not intended to be a return

72 ↗(On Diff #538958)

done :)

79 ↗(On Diff #538958)

modified

mlir/test/Dialect/Tosa/constant-op-fold.mlir
577

Hey, maybe It wasn't clear enough in the summary of the patch. But, right now I'm allowing this optimization just within integer element type. (there is a check at mlir/lib/Dialect/Tosa/Transforms/TosaReduceConstantOptimization.cpp::117).

would upload a new patch for the APfloat handling (would require some changes also reduceAny/reduceAll won't support the float tensors)

amirBish marked 3 inline comments as done.Jul 29 2023, 11:18 AM

@rsuderman would be glad for your feedback regarding the recent changes

Overall I would move these next to the other constant folders. Looking standing back they are all the same as the other layerwise folders and should be kept together. There is not much value is separating the reduction folders into a separate file. Once thats done I can approve and land.

amirBish updated this revision to Diff 551524.Aug 18 2023, 8:25 AM

Moving the constant reduce optimization to the tosa folders

rsuderman accepted this revision.Aug 31 2023, 10:02 AM
This revision is now accepted and ready to land.Aug 31 2023, 10:02 AM

It's my first commit, could you please commit it for me?
thanks in advance
(https://llvm.org/docs/Phabricator.html#committing-someone-s-change-from-phabricator)
@rsuderman

I'll try to help land this, but the printing for tosa ops changed recently to a new format. Can you rebase this onto the latest to make it easier to land?

amirBish updated this revision to Diff 557177.Sep 21 2023, 7:25 AM

Rebasing into the latest

This revision was automatically updated to reflect the committed changes.