Skip to content

[MLIR][Affine] Fixed crash with invalid reduction op (Issue #64073) #8

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: issue_64073
Choose a base branch
from
Open

[MLIR][Affine] Fixed crash with invalid reduction op (Issue #64073) #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,13 +225,13 @@ class AffineParallelLowering : public OpRewritePattern<AffineParallelOp> {
// initialization of the result values.
Attribute reduction = std::get<0>(pair);
Type resultType = std::get<1>(pair);
std::optional<arith::AtomicRMWKind> reductionOp =
arith::symbolizeAtomicRMWKind(
static_cast<uint64_t>(cast<IntegerAttr>(reduction).getInt()));
assert(reductionOp && "Reduction operation cannot be of None Type");
arith::AtomicRMWKind reductionOpValue = *reductionOp;
identityVals.push_back(
arith::getIdentityValue(reductionOpValue, resultType, rewriter, loc));
arith::AtomicRMWKind reductionOpValue = *(arith::symbolizeAtomicRMWKind(
static_cast<uint64_t>(cast<IntegerAttr>(reduction).getInt())));
auto reductionOp =
arith::getIdentityValue(reductionOpValue, resultType, rewriter, loc);
if (!reductionOp)
return failure();
identityVals.push_back(reductionOp);
}
parOp = rewriter.create<scf::ParallelOp>(
loc, lowerBoundTuple, upperBoundTuple, steps, identityVals,
Expand Down
6 changes: 2 additions & 4 deletions mlir/lib/Dialect/Arith/IR/ArithOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2505,6 +2505,8 @@ Value mlir::arith::getIdentityValue(AtomicRMWKind op, Type resultType,
bool useOnlyFiniteValue) {
auto attr =
getIdentityValueAttr(op, resultType, builder, loc, useOnlyFiniteValue);
Copy link
Author

@SwapnilGhanshyala SwapnilGhanshyala Oct 26, 2023

Choose a reason for hiding this comment

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

Addressing comment on lines 2528 to 2530 regarding deletion of maxnumf and minnumf as reduction op cases:

The 'getIdentityAttr()' method called here returns valid attr iff the op passed is a valid reductionOp else returns nullptr.
This method does not consider 'maxnumf' and 'minnumf' as valid reduction ops. If this is correct, then consequently maxnumf and minnumf cases should be safely removable from the "getReductionOp()", as done in this commit.

if (!attr)
return nullptr;
return builder.create<arith::ConstantOp>(loc, attr);
}

Expand All @@ -2525,10 +2527,6 @@ Value mlir::arith::getReductionOp(AtomicRMWKind op, OpBuilder &builder,
return builder.create<arith::MaximumFOp>(loc, lhs, rhs);
case AtomicRMWKind::minimumf:
return builder.create<arith::MinimumFOp>(loc, lhs, rhs);
case AtomicRMWKind::maxnumf:
return builder.create<arith::MaxNumFOp>(loc, lhs, rhs);
case AtomicRMWKind::minnumf:
return builder.create<arith::MinNumFOp>(loc, lhs, rhs);
case AtomicRMWKind::maxs:
Comment on lines -2528 to 2530
Copy link

@bondhugula bondhugula Oct 26, 2023

Choose a reason for hiding this comment

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

Why is this deleted?

Copy link
Author

@SwapnilGhanshyala SwapnilGhanshyala Oct 26, 2023

Choose a reason for hiding this comment

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

Both maxnumf and minnumf are invalid reduction ops and should not be a valid case in getReductionOps.

Copy link

@bondhugula bondhugula Oct 26, 2023

Choose a reason for hiding this comment

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

I see.

return builder.create<arith::MaxSIOp>(loc, lhs, rhs);
case AtomicRMWKind::mins:
Expand Down