[mlir][Arith] Specify evaluation order of getExpr
#87859
Merged
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
The C++ standard does not specify an evaluation order for addition/... operands. E.g., in `a() + b()`, the compiler is free to evaluate `a` or `b` first. This lead to different `mlir-opt` outputs in #85895. (FileCheck passed when compiled with LLVM but failed when compiled with gcc.)
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesThe C++ standard does not specify an evaluation order for addition/... operands. E.g., in This lead to different Full diff: https://github.com/llvm/llvm-project/pull/87859.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp b/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
index 9c6b50e767ea26..90895e381c74b5 100644
--- a/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
+++ b/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
@@ -24,8 +24,15 @@ struct AddIOpInterface
auto addIOp = cast<AddIOp>(op);
assert(value == addIOp.getResult() && "invalid value");
- cstr.bound(value) ==
- cstr.getExpr(addIOp.getLhs()) + cstr.getExpr(addIOp.getRhs());
+ // Note: `getExpr` has a side effect: it may add a new column to the
+ // constraint system. The evaluation order of addition operands is
+ // unspecified in C++. To make sure that all compilers produce the exact
+ // same results (that can be FileCheck'd), it is important that `getExpr`
+ // is called first and assigned to temporary variables, and the addition
+ // is performed afterwards.
+ AffineExpr lhs = cstr.getExpr(addIOp.getLhs());
+ AffineExpr rhs = cstr.getExpr(addIOp.getRhs());
+ cstr.bound(value) == lhs + rhs;
}
};
@@ -49,8 +56,9 @@ struct SubIOpInterface
auto subIOp = cast<SubIOp>(op);
assert(value == subIOp.getResult() && "invalid value");
- cstr.bound(value) ==
- cstr.getExpr(subIOp.getLhs()) - cstr.getExpr(subIOp.getRhs());
+ AffineExpr lhs = cstr.getExpr(subIOp.getLhs());
+ AffineExpr rhs = cstr.getExpr(subIOp.getRhs());
+ cstr.bound(value) == lhs - rhs;
}
};
@@ -61,8 +69,9 @@ struct MulIOpInterface
auto mulIOp = cast<MulIOp>(op);
assert(value == mulIOp.getResult() && "invalid value");
- cstr.bound(value) ==
- cstr.getExpr(mulIOp.getLhs()) * cstr.getExpr(mulIOp.getRhs());
+ AffineExpr lhs = cstr.getExpr(mulIOp.getLhs());
+ AffineExpr rhs = cstr.getExpr(mulIOp.getRhs());
+ cstr.bound(value) == lhs *rhs;
}
};
|
|
@llvm/pr-subscribers-mlir-arith Author: Matthias Springer (matthias-springer) ChangesThe C++ standard does not specify an evaluation order for addition/... operands. E.g., in This lead to different Full diff: https://github.com/llvm/llvm-project/pull/87859.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp b/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
index 9c6b50e767ea26..90895e381c74b5 100644
--- a/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
+++ b/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
@@ -24,8 +24,15 @@ struct AddIOpInterface
auto addIOp = cast<AddIOp>(op);
assert(value == addIOp.getResult() && "invalid value");
- cstr.bound(value) ==
- cstr.getExpr(addIOp.getLhs()) + cstr.getExpr(addIOp.getRhs());
+ // Note: `getExpr` has a side effect: it may add a new column to the
+ // constraint system. The evaluation order of addition operands is
+ // unspecified in C++. To make sure that all compilers produce the exact
+ // same results (that can be FileCheck'd), it is important that `getExpr`
+ // is called first and assigned to temporary variables, and the addition
+ // is performed afterwards.
+ AffineExpr lhs = cstr.getExpr(addIOp.getLhs());
+ AffineExpr rhs = cstr.getExpr(addIOp.getRhs());
+ cstr.bound(value) == lhs + rhs;
}
};
@@ -49,8 +56,9 @@ struct SubIOpInterface
auto subIOp = cast<SubIOp>(op);
assert(value == subIOp.getResult() && "invalid value");
- cstr.bound(value) ==
- cstr.getExpr(subIOp.getLhs()) - cstr.getExpr(subIOp.getRhs());
+ AffineExpr lhs = cstr.getExpr(subIOp.getLhs());
+ AffineExpr rhs = cstr.getExpr(subIOp.getRhs());
+ cstr.bound(value) == lhs - rhs;
}
};
@@ -61,8 +69,9 @@ struct MulIOpInterface
auto mulIOp = cast<MulIOp>(op);
assert(value == mulIOp.getResult() && "invalid value");
- cstr.bound(value) ==
- cstr.getExpr(mulIOp.getLhs()) * cstr.getExpr(mulIOp.getRhs());
+ AffineExpr lhs = cstr.getExpr(mulIOp.getLhs());
+ AffineExpr rhs = cstr.getExpr(mulIOp.getRhs());
+ cstr.bound(value) == lhs *rhs;
}
};
|
matthias-springer
deleted the
users/matthias-springer/disambiguate_get_expr_calls
branch
matthias-springer
added a commit
that referenced
this pull request
Apr 6, 2024
…87860) This commit adds support for `scf.if` to `ValueBoundsConstraintSet`. Example: ``` %0 = scf.if ... -> index { scf.yield %a : index } else { scf.yield %b : index } ``` The following constraints hold for %0: * %0 >= min(%a, %b) * %0 <= max(%a, %b) Such constraints cannot be added to the constraint set; min/max is not supported by `IntegerRelation`. However, if we know which one of %a and %b is larger, we can add constraints for %0. E.g., if %a <= %b: * %0 >= %a * %0 <= %b This commit required a few minor changes to the `ValueBoundsConstraintSet` infrastructure, so that values can be compared while we are still in the process of traversing the IR/adding constraints. Note: This is a re-upload of #85895, which was reverted. The bug that caused the failure was fixed in #87859.
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.
The C++ standard does not specify an evaluation order for addition/... operands. E.g., in
a() + b(), the compiler is free to evaluateaorbfirst.This lead to different
mlir-optoutputs in #85895. (FileCheck passed when compiled with LLVM but failed when compiled with gcc.)