Skip to content

[mlir][tosa] Use typeConverter->convertType<T> #150578

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 4 commits into from
Aug 4, 2025
Merged

Conversation

CoTinker
Copy link
Contributor

Since resultTy might be nullptr, we should use dyn_cast instead of cast. Additionally, typeConverter->convertType<T> is more appropriate in this context.

Since `resultTy` might be nullptr, we should use `dyn_cast` instead of `cast`. Additionally, `typeConverter->convertType<T>` is more appropriate in this context.
@llvmbot
Copy link
Member

llvmbot commented Jul 25, 2025

@llvm/pr-subscribers-mlir-linalg
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-tosa

Author: Longsheng Mou (CoTinker)

Changes

Since resultTy might be nullptr, we should use dyn_cast instead of cast. Additionally, typeConverter-&gt;convertType&lt;T&gt; is more appropriate in this context.


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

1 Files Affected:

  • (modified) mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp (+1-1)
diff --git a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
index 3a205246ddd9e..b7fb7a18c7714 100644
--- a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
+++ b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
@@ -742,7 +742,7 @@ class MaxPool2dConverter : public OpConversionPattern<tosa::MaxPool2dOp> {
 
     bool isUnsigned = op.getType().getElementType().isUnsignedInteger();
     ShapedType resultTy =
-        cast<ShapedType>(getTypeConverter()->convertType(op.getType()));
+        getTypeConverter()->convertType<ShapedType>(op.getType());
     if (!resultTy)
       return rewriter.notifyMatchFailure(op, "failed to convert type");
     Type resultETy = inputTy.getElementType();

@CoTinker CoTinker requested a review from FranklandJack July 28, 2025 08:24
Copy link
Contributor

@FranklandJack FranklandJack left a comment

Choose a reason for hiding this comment

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

Is this really an NFC if we have gone from a static cast to a dynamic one? If not, could we add a test to exercise this change?

@CoTinker
Copy link
Contributor Author

CoTinker commented Jul 28, 2025

Is this really an NFC if we have gone from a static cast to a dynamic one? If not, could we add a test to exercise this change?

Yea, you are right. But can't build a test now for that typeConvert not return nullptr.

void mlir::tosa::populateTosaTypeConversion(TypeConverter &converter) {
converter.addConversion([&](Type type) -> std::optional<Type> {
if (type.isUnsignedInteger()) {
return IntegerType::get(type.getContext(), type.getIntOrFloatBitWidth(),
IntegerType::SignednessSemantics::Signless);
}
return type;
});
converter.addConversion([&](TensorType type) -> std::optional<Type> {
auto converted = converter.convertType(type.getElementType());
if (!converted)
return {};
return type.clone(converted);
});

@CoTinker CoTinker changed the title [mlir][tosa][linalg] Use typeConverter->convertType<T> (NFC) [mlir][tosa][linalg] Use typeConverter->convertType<T> Jul 28, 2025
@CoTinker CoTinker changed the title [mlir][tosa][linalg] Use typeConverter->convertType<T> [mlir][tosa] Use typeConverter->convertType<T> Jul 28, 2025
@CoTinker
Copy link
Contributor Author

CoTinker commented Aug 4, 2025

Hi @lhutton1 , could you please take a look at this PR? thanks.

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.

LGTM, thanks!

@CoTinker
Copy link
Contributor Author

CoTinker commented Aug 4, 2025

Thanks for your review.

@CoTinker CoTinker merged commit f1ca88c into llvm:main Aug 4, 2025
9 checks passed
@CoTinker CoTinker deleted the convert branch August 4, 2025 09:28
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.

4 participants