[mlir] Guard sccp pass from crashing with different source type #120656
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lewuathe
requested review from
banach-space,
dcaballe,
nicolasvasilache and
Groverkss
as code owners
|
@llvm/pr-subscribers-mlir-vector @llvm/pr-subscribers-mlir Author: Kai Sasaki (Lewuathe) ChangesVector::BroadCastOp expects the identical element type in folding. It causes the crash if the different source type is given to the SCCP pass. We need to guard the pass from crashing if the nonidentical element type is given, but still compatible. (e.g. index vs integer type) Full diff: https://github.com/llvm/llvm-project/pull/120656.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index ad709813c6216a..4db3ba239eeaf3 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -2523,8 +2523,16 @@ OpFoldResult BroadcastOp::fold(FoldAdaptor adaptor) {
if (!adaptor.getSource())
return {};
auto vectorType = getResultVectorType();
- if (llvm::isa<IntegerAttr, FloatAttr>(adaptor.getSource()))
- return DenseElementsAttr::get(vectorType, adaptor.getSource());
+ if (auto attr = llvm::dyn_cast<IntegerAttr>(adaptor.getSource())) {
+ if (vectorType.getElementType() != attr.getType())
+ return {};
+ return DenseElementsAttr::get(vectorType, attr);
+ }
+ if (auto attr = llvm::dyn_cast<FloatAttr>(adaptor.getSource())) {
+ if (vectorType.getElementType() != attr.getType())
+ return {};
+ return DenseElementsAttr::get(vectorType, attr);
+ }
if (auto attr = llvm::dyn_cast<SplatElementsAttr>(adaptor.getSource()))
return DenseElementsAttr::get(vectorType, attr.getSplatValue<Attribute>());
return {};
diff --git a/mlir/test/Transforms/sccp.mlir b/mlir/test/Transforms/sccp.mlir
index dcae052c29c248..035fc5c21a38ef 100644
--- a/mlir/test/Transforms/sccp.mlir
+++ b/mlir/test/Transforms/sccp.mlir
@@ -246,3 +246,16 @@ func.func @op_with_region() -> (i32) {
^b:
return %1 : i32
}
+
+// CHECK-LABEL: no_crash_with_different_source_type
+func.func @no_crash_with_different_source_type() {
+ // CHECK: llvm.mlir.constant(0 : index) : i64
+ %0 = llvm.mlir.constant(0 : index) : i64
+ llvm.br ^b1(%0 : i64)
+^b1(%1: i64):
+ llvm.br ^b2
+^b2:
+ // CHECK: vector.broadcast %[[CST:.*]] : i64 to vector<128xi64>
+ %2 = vector.broadcast %1 : i64 to vector<128xi64>
+ llvm.return
+}
|
cxy-1993
requested changes
Dec 20, 2024
| if (vectorType.getElementType() != attr.getType()) | ||
| return {}; | ||
| return DenseElementsAttr::get(vectorType, attr); | ||
| } |
There was a problem hiding this comment.
What is the reason for separating the handling of int and float types?
There was a problem hiding this comment.
dyn_cast cannot take multiple arguments to be casted.
if (auto attr = (llvm::dyn_cast<IntegerAttr>(adaptor.getSource()) || llvm::dyn_cast<FloatAttr>(adaptor.getSource()))) {
Even if I unioned the result of the cast like this, it causes the following error.
llvm-project/mlir/lib/Dialect/Vector/IR/VectorOps.cpp:2527:44: error: member reference base type 'bool' is not a structure or union
if (vectorType.getElementType() != attr.getType())
So we can only separate the logic to handle int and float separetely.
Vector::BroadCastOp expects the idential element type in folding. It causes the crash if the different source type is given to the SCCP pass. We need to guard the pass from crashing if the non-idential element type is given, but still compatible. (e.g. index vs integer type)
Lewuathe
requested review from
rupprecht,
keith,
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci,
yota9,
ftynse,
makslevental,
stellaraccident,
grypp,
aartbik,
PeimingLiu,
yinying-lisa-li,
matthias-springer,
antiagainst and
kuhar
as code owners
Lewuathe
requested review from
hanhanW,
MaheshRavishankar,
rengolin,
JDevlieghere,
lanza,
bcardosolopes,
nikic and
a team
as code owners
Lewuathe
force-pushed
the
fix-crash-with-sccp-pass
branch
from
c062637 to
ade75d1
Compare
ldionne
removed request for
a team,
dcci,
hanhanW,
bcardosolopes,
maksfb,
rafaelauler,
nicolasvasilache,
lanza,
yota9,
dcaballe,
PeimingLiu,
aartbik,
ayermolo,
Groverkss and
yinying-lisa-li
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Vector::BroadCastOp expects the identical element type in folding. It causes the crash if the different source type is given to the SCCP pass. We need to guard the pass from crashing if the nonidentical element type is given, but still compatible. (e.g. index vs integer type)
#120193