[MLIR] Fix assert expressions #112474
Merged
[MLIR] Fix assert expressions #112474
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
apivovarov
requested review from
banach-space,
dcaballe,
hanhanW and
nicolasvasilache
as code owners
apivovarov
requested review from
ftynse
and removed request for
banach-space,
dcaballe,
hanhanW and
nicolasvasilache
Member
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-linalg Author: Alexander Pivovarov (apivovarov) ChangesI noticed that several assertions in MLIR codebase have issues with operator precedence The issue with operator precedence in these assertions is due to the way logical operators are evaluated. The We should add parentheses around the entire expression involving Full diff: https://github.com/llvm/llvm-project/pull/112474.diff 3 Files Affected:
diff --git a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
index e628fb152b52f8..0d6ff2fd908db9 100644
--- a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
+++ b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
@@ -892,8 +892,8 @@ FlatLinearValueConstraints::FlatLinearValueConstraints(IntegerSet set,
set.getNumDims() + set.getNumSymbols() + 1,
set.getNumDims(), set.getNumSymbols(),
/*numLocals=*/0) {
- assert(operands.empty() ||
- set.getNumInputs() == operands.size() && "operand count mismatch");
+ assert((operands.empty() || set.getNumInputs() == operands.size()) &&
+ "operand count mismatch");
// Set the values for the non-local variables.
for (unsigned i = 0, e = operands.size(); i < e; ++i)
setValue(i, operands[i]);
diff --git a/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp b/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
index 09c6b2683b4388..635273bcbc0208 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
@@ -840,11 +840,11 @@ enum VectorMemoryAccessKind { ScalarBroadcast, Contiguous, Gather };
/// TODO: Statically shaped loops + vector masking
static uint64_t getTrailingNonUnitLoopDimIdx(LinalgOp linalgOp) {
SmallVector<int64_t> loopRanges = linalgOp.getStaticLoopRanges();
- assert(linalgOp.hasDynamicShape() ||
- llvm::count_if(loopRanges, [](int64_t dim) { return dim != 1; }) ==
- 1 &&
- "For statically shaped Linalg Ops, only one "
- "non-unit loop dim is expected");
+ assert(
+ (linalgOp.hasDynamicShape() ||
+ llvm::count_if(loopRanges, [](int64_t dim) { return dim != 1; }) == 1) &&
+ "For statically shaped Linalg Ops, only one "
+ "non-unit loop dim is expected");
size_t idx = loopRanges.size() - 1;
for (; idx >= 0; idx--)
diff --git a/mlir/lib/Dialect/Tensor/Utils/Utils.cpp b/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
index e0b91f323b0e64..5c16e538ac2420 100644
--- a/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
+++ b/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
@@ -27,9 +27,9 @@ PadOp mlir::tensor::createPadHighOp(RankedTensorType resType, Value source,
OpBuilder &b,
SmallVector<Value> dynOutDims) {
- assert((resType.getNumDynamicDims() == dynOutDims.size()) ||
- dynOutDims.empty() &&
- "Either none or all output dynamic dims must be specified!");
+ assert(((resType.getNumDynamicDims() == dynOutDims.size()) ||
+ dynOutDims.empty()) &&
+ "Either none or all output dynamic dims must be specified!");
// Init "low" and "high" padding values ("low" is kept as is, "high" is
// computed below).
|
Member
|
@llvm/pr-subscribers-mlir-tensor Author: Alexander Pivovarov (apivovarov) ChangesI noticed that several assertions in MLIR codebase have issues with operator precedence The issue with operator precedence in these assertions is due to the way logical operators are evaluated. The We should add parentheses around the entire expression involving Full diff: https://github.com/llvm/llvm-project/pull/112474.diff 3 Files Affected:
diff --git a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
index e628fb152b52f8..0d6ff2fd908db9 100644
--- a/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
+++ b/mlir/lib/Analysis/FlatLinearValueConstraints.cpp
@@ -892,8 +892,8 @@ FlatLinearValueConstraints::FlatLinearValueConstraints(IntegerSet set,
set.getNumDims() + set.getNumSymbols() + 1,
set.getNumDims(), set.getNumSymbols(),
/*numLocals=*/0) {
- assert(operands.empty() ||
- set.getNumInputs() == operands.size() && "operand count mismatch");
+ assert((operands.empty() || set.getNumInputs() == operands.size()) &&
+ "operand count mismatch");
// Set the values for the non-local variables.
for (unsigned i = 0, e = operands.size(); i < e; ++i)
setValue(i, operands[i]);
diff --git a/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp b/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
index 09c6b2683b4388..635273bcbc0208 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
@@ -840,11 +840,11 @@ enum VectorMemoryAccessKind { ScalarBroadcast, Contiguous, Gather };
/// TODO: Statically shaped loops + vector masking
static uint64_t getTrailingNonUnitLoopDimIdx(LinalgOp linalgOp) {
SmallVector<int64_t> loopRanges = linalgOp.getStaticLoopRanges();
- assert(linalgOp.hasDynamicShape() ||
- llvm::count_if(loopRanges, [](int64_t dim) { return dim != 1; }) ==
- 1 &&
- "For statically shaped Linalg Ops, only one "
- "non-unit loop dim is expected");
+ assert(
+ (linalgOp.hasDynamicShape() ||
+ llvm::count_if(loopRanges, [](int64_t dim) { return dim != 1; }) == 1) &&
+ "For statically shaped Linalg Ops, only one "
+ "non-unit loop dim is expected");
size_t idx = loopRanges.size() - 1;
for (; idx >= 0; idx--)
diff --git a/mlir/lib/Dialect/Tensor/Utils/Utils.cpp b/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
index e0b91f323b0e64..5c16e538ac2420 100644
--- a/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
+++ b/mlir/lib/Dialect/Tensor/Utils/Utils.cpp
@@ -27,9 +27,9 @@ PadOp mlir::tensor::createPadHighOp(RankedTensorType resType, Value source,
OpBuilder &b,
SmallVector<Value> dynOutDims) {
- assert((resType.getNumDynamicDims() == dynOutDims.size()) ||
- dynOutDims.empty() &&
- "Either none or all output dynamic dims must be specified!");
+ assert(((resType.getNumDynamicDims() == dynOutDims.size()) ||
+ dynOutDims.empty()) &&
+ "Either none or all output dynamic dims must be specified!");
// Init "low" and "high" padding values ("low" is kept as is, "high" is
// computed below).
|
Contributor
|
LGTM, thanks for the fix! |
hanhanW
approved these changes
Oct 16, 2024
dcaballe
approved these changes
Oct 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
I noticed that several assertions in MLIR codebase have issues with operator precedence
The issue with operator precedence in these assertions is due to the way logical operators are evaluated. The
&&operator has higher precedence than the||operator, which means the assertion is currently evaluating incorrectly, like this:We should add parentheses around the entire expression involving
dynOutDims.empty()to ensure that the logical conditions are grouped correctly. Here’s the corrected version: