-
Notifications
You must be signed in to change notification settings - Fork 45
Guard LogSimplify rewrites against domain narrowing (#2570) #2576
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
wsmoses
merged 15 commits into
EnzymeAD:main
from
kimjune01:fix/logsimplify-domain-guards-2570
Jul 23, 2026
Merged
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
86eef28
Guard LogSimplify rewrites against domain narrowing (#2570)
kimjune01 4f2c333
Guard LogSimplify rewrites against domain narrowing (#2570)
kimjune01 ba6fd78
Fix logsimplify lit test: expect non_negative memo attr; clang-format
kimjune01 ab6cee3
Merge fork branch before pushing lit/format fix
kimjune01 9cc0a7b
Tighten LogSimplify domain guards
kimjune01 e57a9da
Keep LogSimplify predicate local
kimjune01 9bab829
clang-format: collapse LogSimplify predicate line
kimjune01 6b6f1e5
LogSimplify: gate log(a*a) rewrite on non-negativity instead of abs
kimjune01 aa06597
Rename isValidLogSplitConstant to isPositiveFiniteConstant
kimjune01 b76081e
Rename log-split constant guard to isPositiveNonNanConstant
kimjune01 51ec6b6
Make isPositiveNonNanConstant a static member of LogSimplify
kimjune01 ff1c40f
Trim LogSimplify guard comments
kimjune01 48aaf2b
Update main2 CHECK for non_negative annotation on squared multiply
kimjune01 5660c46
LogSimplify: exclude infinite constants from log-splits
kimjune01 b30b8d7
Expect non_negative stamp on abs in main_square_nonneg CHECK
kimjune01 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27750,6 +27750,19 @@ struct LogSimplify final | |
|
|
||
| LogicalResult matchAndRewriteImpl(stablehlo::LogOp op, | ||
| PatternRewriter &rewriter) const { | ||
| auto isValidLogSplitConstant = [](Value value, bool allowZero) { | ||
| DenseElementsAttr attr; | ||
| if (!matchPattern(value, m_Constant(&attr)) || | ||
| !isa<FloatType>(attr.getElementType())) | ||
| return false; | ||
|
|
||
| return llvm::all_of(attr.getValues<APFloat>(), | ||
| [allowZero](const APFloat &element) { | ||
| return !element.isNaN() && !element.isNegative() && | ||
| (allowZero || !element.isZero()); | ||
| }); | ||
| }; | ||
|
|
||
| { // log(exp(x)) -> x | ||
| auto defOp = op.getOperand().getDefiningOp<stablehlo::ExpOp>(); | ||
| if (defOp) { | ||
|
|
@@ -27758,22 +27771,17 @@ struct LogSimplify final | |
| } | ||
| } | ||
|
|
||
| { // log(pow(x, y)) -> y * log(x) | ||
| auto defOp = op.getOperand().getDefiningOp<stablehlo::PowOp>(); | ||
| if (defOp) { | ||
| rewriter.replaceOpWithNewOp<stablehlo::MulOp>( | ||
| op, defOp.getRhs(), | ||
| stablehlo::LogOp::create(rewriter, op.getLoc(), defOp.getLhs())); | ||
| return success(); | ||
| } | ||
| } | ||
|
|
||
| { | ||
| auto defOp = op.getOperand().getDefiningOp<stablehlo::MulOp>(); | ||
| if (defOp) { | ||
| auto lhs = defOp.getLhs(); | ||
| auto rhs = defOp.getRhs(); | ||
| if (lhs == rhs) { // log(mul(a, a)) -> 2 * log(a) | ||
| // log(mul(a, a)) -> 2 * log(a) when a is provably non-negative. | ||
| // For a < 0, 2 * log(a) is NaN even though log(a*a) is finite, so we | ||
| // only fire this when a is provably non-negative. Gating on the | ||
| // non-negative analysis keeps the rewrite correct without inserting an | ||
| // abs (which may be slower than the mul). See issue #2570. | ||
| if (lhs == rhs && guaranteedNonNegativeResult(lhs, rewriter)) { | ||
| rewriter.replaceOpWithNewOp<stablehlo::MulOp>( | ||
| op, | ||
| stablehlo::ConstantOp::create( | ||
|
|
@@ -27786,10 +27794,16 @@ struct LogSimplify final | |
| if (anyOperandIsConstant(defOp) && | ||
| !allOperandsAreConstant(defOp)) { // log(mul(a, b)) -> log(a) + | ||
| // log(b) if a or b is constant | ||
| rewriter.replaceOpWithNewOp<stablehlo::AddOp>( | ||
| op, stablehlo::LogOp::create(rewriter, op.getLoc(), lhs), | ||
| stablehlo::LogOp::create(rewriter, op.getLoc(), rhs)); | ||
| return success(); | ||
| // Require a strictly positive constant to avoid narrowing the | ||
| // domain for negative or zero constants. This does not make the | ||
| // rewrite bit-exact under floating-point rounding. See issue #2570. | ||
| Value cst = matchPattern(lhs, m_Constant()) ? lhs : rhs; | ||
| if (isValidLogSplitConstant(cst, /*allowZero=*/false)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you rename this from isValidLogSplitConstant to isPositiveFiniteConstant for clarity |
||
| rewriter.replaceOpWithNewOp<stablehlo::AddOp>( | ||
| op, stablehlo::LogOp::create(rewriter, op.getLoc(), lhs), | ||
| stablehlo::LogOp::create(rewriter, op.getLoc(), rhs)); | ||
| return success(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -27839,10 +27853,19 @@ struct LogSimplify final | |
| if (anyOperandIsConstant(defOp) && | ||
| !allOperandsAreConstant(defOp)) { // log(div(a, b)) -> log(a) - | ||
| // log(b) if a or b is constant | ||
| rewriter.replaceOpWithNewOp<stablehlo::SubtractOp>( | ||
| op, stablehlo::LogOp::create(rewriter, op.getLoc(), lhs), | ||
| stablehlo::LogOp::create(rewriter, op.getLoc(), rhs)); | ||
| return success(); | ||
| // A constant numerator must be strictly positive; a zero numerator | ||
| // can turn a finite log(-0) into NaN after splitting. A zero | ||
| // denominator is safe here, so it only needs to be non-negative. | ||
| // This does not make the rewrite bit-exact under floating-point | ||
| // rounding. See issue #2570. | ||
| bool constantIsLhs = matchPattern(lhs, m_Constant()); | ||
| Value cst = constantIsLhs ? lhs : rhs; | ||
| if (isValidLogSplitConstant(cst, /*allowZero=*/!constantIsLhs)) { | ||
| rewriter.replaceOpWithNewOp<stablehlo::SubtractOp>( | ||
| op, stablehlo::LogOp::create(rewriter, op.getLoc(), lhs), | ||
| stablehlo::LogOp::create(rewriter, op.getLoc(), rhs)); | ||
| return success(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry one final minor comment, can you make this a static member function rather than a lambda?
it'll be easier to debug in the future [and faster to compile/run]