Repository navigation
Conversation
c17 defines __STDC_IEC_559__, so Annex F binds it: the relational
operators are IEEE 754's signaling predicates and raise FE_INVALID for a
NaN operand (C17 F.9.3), while ==, != and the <math.h> comparison macros
are quiet. Both backends emitted the quiet compare for everything, so
NaN < 1.0 raised nothing.
The IR now says which: FloatCmp { Eq, Ne, Lt(NanCompare), .. } with four
new signaling opcodes (fcmps_olt..fcmps_oge), taken apart through
Opcode::float_cmp everywhere. float_comparison() is the one place a C
operator becomes a float compare. x86-64 emits comis*/fcomip for the
signaling ones, aarch64 fcmpe; binary128 maps them to libgcc's
__lttf2.. and builds a quiet relational as gcc does, the signaling helper
behind an unordered test (has_quiet_relational), which also fixes
isless() and friends raising spuriously on aarch64 long double.
Two places evaluated a comparison on a path the program never runs and
now would raise: ifconv speculated any float compare out of its guard
(`isunordered(x, y) || x < y` raised for a NaN at -O1), and the
linearizer's is_pure_expr treated `c ? x < y : 0` as safe to evaluate
unconditionally. ifconv now speculates a signaling compare only as its
quiet twin, and only where reaching the arm proves the pair ordered
(CmpFacts::proves_ordered); a float relational is no longer pure.
Constant folds of comparisons whose answer no operand can change still
drop the flag, as gcc's do; DECISIONS.md records it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
At the x86-64 baseline the native SIMD table only lowers what one instruction does, so v4si * v4si is four scalar imulls and unsigned 16/32-bit lane comparisons run lane by lane, where gcc uses pmuludq and sign-flipped pcmpgt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Under Annex F the exception flags are observable, but the linearizer's is_pure_expr treated float arithmetic and conversions as pure, so `c ? a * b : 0` became a select and reported an overflow with c == 0 -- at every level, on both targets. Arm conversions to the result type (`c ? n : 0.5f` with an int n, inexact) did the same. One rule now answers it: Opcode::fp_raise() (exhaustive, so a new opcode must be classified) gives Never / May / Conversion, and conversion_raises_fp() decides a conversion by whether the destination format holds every source value (FpFormat::holds, holds_integer). Instruction::may_raise_fp_exception combines them. The linearizer (binary_raises_fp, is_speculatable_arm, casts, inline libm calls) and ifconv both ask it. Exact operations -- fneg, fabs, copysign, signbit, widening, quiet compares -- stay speculatable, and an integer constant arm that converts exactly still becomes a select. The checks apply under -ftrapping-math, the default; -fno-trapping-math says nothing reads the flags, and selects form as in gcc. __builtin_constant_p of an operand that may raise now answers 0 at once: deferring it would run the operand at -O0, where nothing deletes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- inline_asm.rs (both targets): the clobber loop matched every clobber and did nothing, under comments saying clobbers were only documented and the allocator was trusted. Register clobbers are allocator constraints (get_constraint_info) and "memory" makes the asm an IR barrier (is_memory_barrier); the empty loop is gone. - parse/asm.rs: `volatile` is not a "Phase 2 feature" -- every asm is a memory access, so DCE never deletes one and nothing reorders it. - linearize_emit.rs: `%` on floats is not "for now integer modulo"; the parser rejects it (C17 6.5.5p2). - parse/expression.rs: sizeof's type/expression split is not a "simplified check"; it tries a type name and falls back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes correctness-critical compiler codegen and IEEE-754 exception semantics across the IR, optimizer, and both backends, where subtle soundness errors would be hard to detect automatically and warrant human sign-off.
Review effort: Balanced
Findings: None
What changed in this PR
This PR hardens the in-tree C compiler (cc) around IEEE‑754 floating‑point exception semantics. Under Annex F (c17 defines __STDC_IEC_559__), the FE_INVALID/overflow/inexact flags are observable via fetestexcept, so the compiler must (a) distinguish C's signaling relational operators (<, <=, >, >=) from the quiet comparisons (==, !=, the isgreater family), and (b) never evaluate a float operation that can raise on a path the program would not have taken (e.g. turning c ? a*b : 0 into a branch‑free select).
Changes:
- Introduces
NanCompare,FloatCmp, andFpRaiseIR abstractions plus signaling comparison opcodes (FCmpsO{Lt,Le,Gt,Ge}), withconversion_raises_fp/Instruction::may_raise_fp_exceptionandFpFormat::holds*helpers. - Makes speculation exception‑aware: the linearizer's
is_pure_expr/is_speculatable_armandifconvkeep branches for raising ops, whileifconvmay still speculate a signaling compare as its quiet twin where operands are provably ordered (CmpFacts::proves_ordered); backends emitcomis*/fcomip/fcmpevsucomis*/fucomip/fcmpaccordingly. - Adds extensive codegen/IR tests and updates docs (DECISIONS.md, BUILTIN.md, README.md, TODO.md); also tidies stale inline‑asm clobber/
sizeofcomments.
| File | Description |
|---|---|
| cc/ir/mod.rs | New NanCompare/FloatCmp/FpRaise, signaling opcodes, float_cmp/fp_raise/conversion_raises_fp, tests |
| cc/float.rs | FpFormat::holds/holds_integer/holds_integer_value exactness checks + tests |
| cc/ir/linearize.rs | Exception‑aware purity/speculation (binary_raises_fp, converting_raises_fp, is_speculatable_arm), emit_unordered |
| cc/ir/linearize_emit.rs | Extracted binary_opcode; signaling/quiet comparison selection; guarded binary128 quiet relational |
| cc/ir/ifconv.rs | Speculation::relaxes + quiet‑twin rewrite on collapse; run now takes TypeTable |
| cc/ir/facts.rs | CmpFacts::record/proves_ordered to prove operands ordered |
| cc/ir/constfold.rs | Outcomes::of_op routes via float_cmp so signaling pairs fold like quiet twins |
| cc/ir/instcombine.rs | Doc update on folding signaling compares |
| cc/opt.rs | ifconv pass passes c.types |
| cc/arch/x86_64/{float,x87,codegen,lir}.rs | ComiFp/X87CmpPop { nan }, dispatch via float_cmp |
| cc/arch/aarch64/{float,lir,mapping}.rs | Fcmp { nan } (fcmp/fcmpe), binary128 helper mapping + tests |
| cc/arch/mapping.rs | has_quiet_relational; binary128 cmp asserts signaling |
| cc/arch/{x86_64,aarch64}/inline_asm.rs, regalloc.rs | Clobber comment cleanup; doc tweak |
| cc/parse/{asm,expression}.rs | Clarified volatile‑asm / sizeof comments |
| cc/tests/**, cc/test_asm/** | New fp‑compare/fp‑untaken‑arm codegen & asm tests; module registration |
| cc/{DECISIONS,BUILTIN,README,TODO}.md | Documentation of the signaling/quiet and speculation rules |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
test_relational_operators_signal_and_everything_else_is_quiet linearized on Target::host() and expected one quiet FCmpOLt for `isless` on every format, which holds for x87 long double but not for linux-aarch64's binary128 long double: it has no quiet relational and builds `isless` as an unordered test guarding the signaling call (its own test covers that). linux-aarch64 CI failed on it. Run float, double and x87 long double on x86-64, and float and double on aarch64, whatever the host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
At the x86-64 baseline the native SIMD table listed only what one instruction does, so v4si * v4si ran as four scalar imulls and unsigned 16/32-bit lane orders ran lane by lane. SSE2 builds both, as gcc does: - Mul of dwords: pshufd $0xf5 / pmuludq for the even and odd lanes, then (16-byte vectors) pshufd $8 on each and punpckldq to gather the low halves. New PackedOp::MulEvenDwords. - CmpGtU/CmpGeU of words and dwords: the signed pcmpgt form with each operand's sign bits flipped (packed_form's flip_signs), the sign-bit constant built in registers by one helper, emit_sign_bits, that FNeg now shares. Bytes, and every width under -msse4.1, keep the pminu form; -msse4.1 keeps pmulld. The sequences need a third XMM temporary when the target lives in memory: XMM13 (simd::SPARE_XMM). The XMM colouring now forbids it to any value live at such an instruction, operands and result included (spare_xmm_forbidden), and an ms_abi function saves it, since Win64 preserves XMM13. A runtime check keeps 14 vectors live across the sequences; it fails at -O2 without the constraint. Removes the TODO.md entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Whether a `?:` arm's integer constant converts to floating exactly was decided from eval_const_expr's i128, through unsigned_abs. An unsigned __int128 at or above 2^127 is held there as negative, so `(unsigned __int128)-1` read as magnitude 1 and "fit": the arm became a select and converted 2^128 - 1 -- inexact, and an overflow for float -- whatever the condition. Found by code review of de39b4e. FpFormat::holds_integer_value(i128) becomes holds_integer_magnitude(u128), and the linearizer's const_magnitude reads the constant by its type's signedness: the bits for an unsigned type, the absolute value for a signed one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
No description provided.