Fold floating-point comparisons with a constant NaN in morph - #130838
#130838Fold floating-point comparisons with a constant NaN in morph#130838tannergooding merged 4 commits intodotnet:maindotnet/runtime:mainfrom tannergooding:tannergooding-redesigned-spoontannergooding/runtime:tannergooding-redesigned-spoonCopy head branch name to clipboard
Conversation
gtFoldExprSpecial dispatched floating special-folding by the node's result type, but relational operators have an integral result with floating-point operands, so float compares never reached gtFoldExprSpecialFloating and were only folded later in value numbering. Dispatch by operand type instead so the fold happens early and can unblock the inliner and other heuristics. Also fix latent bugs in the now-live block: GT_NE asserted GTF_RELOP_NAN_UN was clear, but bne.un imports as GT_NE with that flag set, and its result was hardcoded true, which is wrong for an ordered '!='. All six relops now fold uniformly to true for unordered comparisons and false for ordered ones. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR fixes a missed JIT folding opportunity for floating-point comparisons against a constant NaN by routing gtFoldExprSpecial to the floating-point special-folding path based on operand type (not result type). This makes x <op> NaN / NaN <op> x fold during morph for relops (whose result type is integral), instead of relying on later value-numbering folding.
Changes:
- Update
Compiler::gtFoldExprSpecialto dispatch togtFoldExprSpecialFloatingwhen operands are floating-point, including for relops. - Revive and correct NaN-folding for all relops in
gtFoldExprSpecialFloating, honoringGTF_RELOP_NAN_UNuniformly (and removing now-invalid asserts / fixingGT_NEbehavior). - Add a directed JIT test project and test that exercises all six relops, both operand orders, and both
float/double.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/gentree.cpp | Dispatch special folding based on floating operand type; enable/repair NaN folding for relops in morph. |
| src/tests/JIT/Directed/ConstantFolding/FloatNaNCompareFolding.csproj | Adds a directed test project for NaN-compare folding coverage. |
| src/tests/JIT/Directed/ConstantFolding/FloatNaNCompareFolding.cs | New directed regression test covering all relops and operand orders for float/double. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
CC. @dotnet/jit-contrib, @EgorBo. Bug fix in floating-point constant folding. Doesn't need a backport as the old logic never actually was hit |
gtFoldExprSpecial routes float-operand binops to gtFoldExprSpecialFloating by op1 type. A GT_COMMA has heterogeneously-typed operands, so a floating discarded op1 paired with a non-floating value op2 was misrouted into the floating helper and tripped its entry assert during crossgen. A COMMA has no fold in this one-const path (its fully-constant form still folds to op2 via gtFoldExprBinaryConst), so bail before the dispatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "aae0e5c044cd6ce17906186a47b26b20f1a0a314",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f2281d59e62aa6a75e53055b8f842bac108b9887",
"last_reviewed_commit": "aae0e5c044cd6ce17906186a47b26b20f1a0a314",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f2281d59e62aa6a75e53055b8f842bac108b9887",
"last_recorded_worker_run_id": "29688133375",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "aae0e5c044cd6ce17906186a47b26b20f1a0a314",
"review_id": 4730815465
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: gtFoldExprSpecial dispatched to gtFoldExprSpecialFloating based on the node's result type. Because relational operators produce a TYP_INT result over floating operands, the relop arms of the floating helper were provably dead — comparisons with a constant NaN were only folded later during value numbering, leaving early-morph heuristics (e.g. the inliner) blind to the constant result. The motivation is sound and well-explained.
Approach: Dispatch by operand type (varTypeIsFloating(op1)) instead of result type so x <op> NaN / NaN <op> x folds during morph. Reviving the previously-dead relop arms surfaced two latent bugs, both fixed: the GT_NE/GT_EQ arms asserted (gtFlags & GTF_RELOP_NAN_UN) == 0 — which would fire immediately since bne.un imports as GT_NE with that flag set (and gtReverseCond flips it for float relops) — and the GT_NE arm hardcoded the result to true, wrong for an ordered !=. All six relops now fold uniformly (ordered NaN compare = false, unordered = true), matching both the two-constant path gtFoldExprBinaryConstDbl and the value-numbering path. A GTF_RELOP_JMP_USED + side-effect guard was added to preserve the JTRUE(relop) invariant some phases depend on, and a GT_COMMA early-bail prevents the new operand-type dispatch from misrouting a COMMA based on its op1's unrelated type.
Summary: LGTM. The change is small, self-contained (src/coreclr/jit/gentree.cpp plus a new directed test), and the reasoning is carefully documented. I verified the folding semantics are consistent with gtFoldExprBinaryConstDbl (lines 18204-18220) and the VN path, that the removed asserts were genuinely incorrect, and that the new dispatch cannot misroute non-compare binops (both operands are floating for the reachable ops, as the restored assert confirms). The regression test exercises all six operators, both operand orders, float/double, and the unordered branch imports (bne.un/bge.un/...) that carry GTF_RELOP_NAN_UN — precisely the path that would have tripped the removed GT_NE assert in a checked build.
Detailed Findings
No blocking issues.
Minor, non-blocking observations (offered for consideration only):
- Test asserts correctness, not that folding occurred.
FloatNaNCompareFolding.csvalidates the observable comparison results, which are semantically correct with or without the morph-time fold. Its real regression value is guarding against the reintroduced asserts (which would fire in checked builds forbne.un) and the previous incorrectGT_NE-returns-truebehavior. That is reasonable coverage for a directed test; a disassembly-style check would be the only way to prove the fold now happens in morph, which is typically out of scope for these directed tests. No change requested. - Redundant
GT_CASTfilter insidegtFoldExprSpecialFloating. With the new caller-side dispatch,gtFoldExprSpecialalready returns early forGT_CASTbefore reaching the floating dispatch, so theGT_CASTguard inside the floating helper is now unreachable from that path. Harmless and arguably good defense-in-depth given the helper is also invoked directly; noting only for awareness.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 118.3 AIC · ⌖ 10.5 AIC · ⊞ 10K
EgorBo
left a comment
There was a problem hiding this comment.
LGTM with a question about GTF_RELOP_JMP_USED
…lineData Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@EgorBo, this one also needs re-sign off. This was the first time we used nan/inf as theory inputs in the JIT generated wrapper tests, so it required a test handler be updated to account for that. |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/tests/JIT/Directed/ConstantFolding/FloatNaNCompareFolding.cs:12
- The header comment mixes ordered vs unordered NaN semantics (it says "ordered" but then treats
!=as always true). Since this test is specifically exercisingGTF_RELOP_NAN_UNimports, consider rewording to explicitly describe ordered vs unordered behavior to avoid misleading future readers.
// Regression coverage for folding floating-point comparisons against a constant
// NaN in gtFoldExprSpecialFloating. Every ordered comparison with NaN is false and
// '!=' is true. The other operand is an opaque argument, so the fold goes through
// the special (variable operand + constant NaN) path rather than full constant
// folding. The branch helpers additionally exercise the unordered relop imports
// (bne.un / bge.un / ...) that carry GTF_RELOP_NAN_UN.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
gtFoldExprSpecialdispatched the floating-point special-folding helper based on the node's result type. Relational operators (GT_EQ/GT_NE/GT_LT/GT_LE/GT_GT/GT_GE) have an integral (TYP_INT) result with floating-point operands, so a float compare never reachedgtFoldExprSpecialFloating— the relop arms of that helper were provably dead (it even assertedvarTypeIsFloating(tree->TypeGet())on entry). Comparisons against a constant NaN were therefore only folded later, in value numbering.This dispatches by operand type instead, so
x <op> NaN/NaN <op> xfolds during morph. Folding these early can unblock the inliner and other heuristics that run before VN.Reviving that block surfaced two latent bugs in it, now fixed:
GT_NEasserted(gtFlags & GTF_RELOP_NAN_UN) == 0, butbne.unimports asGT_NEwith that flag set (andgtReverseCondflips it for float relops), so the assert would fire the moment the code went live.GT_NEarm hardcoded the result totrue, which is wrong for an ordered!=.All six relops now fold uniformly: a comparison with NaN is
truewhenGTF_RELOP_NAN_UNis set (unordered) andfalseotherwise (ordered), matchinggtFoldExprBinaryConstDbland the value-numbering path.Added a directed regression test covering all six operators, both operand orders, and
float/double.Note
This PR was authored with the assistance of GitHub Copilot.