Skip to content

[mlir][tosa] Fix ability to expand ranks with dynamic shape support #128037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

Jerry-Ge
Copy link
Member

@Jerry-Ge Jerry-Ge commented Feb 20, 2025

  • Fix ability to expand ranks with dynamic shape support
  • Simplify the code

@llvmbot
Copy link
Member

llvmbot commented Feb 20, 2025

@llvm/pr-subscribers-mlir-tosa

@llvm/pr-subscribers-mlir

Author: Jerry-Ge (Jerry-Ge)

Changes

Fix ability to expand ranks with dynamic shape support


Full diff: https://github.com/llvm/llvm-project/pull/128037.diff

1 Files Affected:

  • (modified) mlir/lib/Dialect/Tosa/Utils/ConversionUtils.cpp (+2-2)
diff --git a/mlir/lib/Dialect/Tosa/Utils/ConversionUtils.cpp b/mlir/lib/Dialect/Tosa/Utils/ConversionUtils.cpp
index d1a8732dac212..9ade05c8f095b 100644
--- a/mlir/lib/Dialect/Tosa/Utils/ConversionUtils.cpp
+++ b/mlir/lib/Dialect/Tosa/Utils/ConversionUtils.cpp
@@ -88,9 +88,9 @@ computeReshapeOutput(ArrayRef<int64_t> higherRankShape,
     higherRankDim = higherRankShape[i];
     lowerRankDim = lowerRankShape[j];
 
-    if (lowerRankDim == 1 && higherRankDim > 1)
+    if (lowerRankDim == 1 && higherRankDim != 1)
       reshapeOutputShape[i] = 1;
-    else if ((lowerRankDim > 1 && higherRankDim == 1) ||
+    else if ((lowerRankDim != 1 && higherRankDim == 1) ||
              (lowerRankDim == higherRankDim))
       reshapeOutputShape[i] = lowerRankDim;
     else if (higherRankDim != lowerRankDim)

Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to test this change at all?

@Jerry-Ge
Copy link
Member Author

Is it possible to test this change at all?

good question, i don't have a clear answer. CC the original author: @sjarus

@Jerry-Ge Jerry-Ge force-pushed the expand_ranks branch 2 times, most recently from 9f7d82f to 16462f4 Compare February 24, 2025 22:01
- The use of != 1 accommodates the use of kDynamicDim
- Simplified the for loop to iterate only on lowerRank and access the
  higherRank dim by using the rankDiff

Signed-off-by: Suraj Sudhir <[email protected]>
Change-Id: I0f223f335667b2e32c43d4370f0a4b11b0617694
@Jerry-Ge Jerry-Ge merged commit 0a7809c into llvm:main Feb 25, 2025
9 of 10 checks passed
@Jerry-Ge Jerry-Ge deleted the expand_ranks branch March 20, 2025 21:41
lhutton1 pushed a commit to lhutton1/llvm-project that referenced this pull request Jun 11, 2025
…port to tosa_arm_internal

This patch merges down upstream review changes into
mltech/tosa_arm_internal branch.

Upstream Github patch: llvm#128037

- The use of != 1 accommodates the use of kDynamicDim
- Simplified the for loop to iterate only on lowerRank and access the
  higherRank dim by using the rankDiff

Signed-off-by: Suraj Sudhir <[email protected]>
Change-Id: Ia011099171b0965d5deeb06645e8d3731f980915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants