Skip to content

[PowerPC] fix ppc_fp128 FABS miscompile #209286

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.

Sign up for GitHub

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

Open
folkertdev wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[PowerPC] fix ppc_fp128 FABS miscompile #209286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2d5d6e1
[PowerPC] fix `ppc_fp128` `FABS` bug
folkertdev Jul 13, 2026
74db7a4
nits and add test
folkertdev Jul 30, 2026
d6d6fb5
fix endiannes bug in `AssertNoFPClass` too
folkertdev Jul 30, 2026
3070f84
check that sign knowledge is used on LE too
folkertdev Jul 30, 2026
fa75252
fix `copysign` for `ppc_fp128`
folkertdev Jul 31, 2026
5c894a9
Merge remote-tracking branch 'upstream/main' into fix-ppc_f128-fabs-bug
folkertdev Jul 31, 2026
f75acfd
Merge remote-tracking branch 'upstream/main' into fix-ppc_f128-fabs-bug
folkertdev Jul 31, 2026
d7b6b74
update copysign tests
folkertdev Jul 31, 2026
42736cc
fix another ppcf128 bug in fneg
folkertdev Jul 31, 2026
efe0d3d
formatting
folkertdev Jul 31, 2026
330b268
Merge remote-tracking branch 'upstream/main' into fix-ppc_f128-fabs-bug
folkertdev Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20346,6 +20346,11 @@ SDValue DAGCombiner::visitFCOPYSIGN(SDNode *N) {
if (VT != N1.getValueType())
return SDValue();

// ppcf128 has two sign bits (in bits 127 and 63), the logic below is invalid.
EVT SVT = VT.getScalarType();
if (!APFloat::hasSignBitInMSB(SVT.getFltSemantics()) || SVT == MVT::ppcf128)
return SDValue();

// If this is equivalent to a disjoint or, replace it with one. This can
// happen if the sign operand is a sign mask (i.e., x << sign_bit_position).
if (DAG.SignBitIsZeroFP(N0) &&
Expand Down
35 changes: 30 additions & 5 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4146,25 +4146,50 @@ KnownBits SelectionDAG::computeKnownBits(SDValue Op, const APInt &DemandedElts,

FPClassTest NoFPClass =
static_cast<FPClassTest>(Op.getConstantOperandVal(1));

EVT SVT = Op.getValueType().getScalarType();
unsigned SignBitPos;
if (SVT == MVT::ppcf128)
// The sign bit position depends on endianness: ppc_fp128 is two doubles
// in a trenchcoat, and it is the high-order double that carries the sign.
SignBitPos = getDataLayout().isBigEndian() ? 127 : 63;
else if (APFloat::hasSignBitInMSB(SVT.getFltSemantics()))
// IEEE-like formats, bf16, x86_fp80: the sign bit is the integer MSB.
SignBitPos = BitWidth - 1;
else
break;

const FPClassTest NegativeTestMask = fcNan | fcNegative;
if ((NoFPClass & NegativeTestMask) == NegativeTestMask) {
// Cannot be negative.
Known.makeNonNegative();
Known.Zero.setBit(SignBitPos);
}

const FPClassTest PositiveTestMask = fcNan | fcPositive;
if ((NoFPClass & PositiveTestMask) == PositiveTestMask) {
// Cannot be positive.
Known.makeNegative();
Known.One.setBit(SignBitPos);
}

break;
}
case ISD::FABS:
// fabs clears the sign bit
case ISD::FABS: {
Known = computeKnownBits(Op.getOperand(0), DemandedElts, Depth + 1);
Known.makeNonNegative();
EVT SVT = Op.getValueType().getScalarType();
if (SVT == MVT::ppcf128) {
// The sign bit position depends on endianness: ppc_fp128 is two doubles
// in a trenchcoat, fabs only clears the sign bit of the high-order
// double.
Known.resetAll();
Known.Zero.setBit(getDataLayout().isBigEndian() ? 127 : 63);
} else if (APFloat::hasSignBitInMSB(SVT.getFltSemantics())) {
// IEEE-like formats, bf16, x86_fp80: the sign bit is the integer MSB,
// fabs clears that sign bit.
Known.makeNonNegative();
Comment thread
folkertdev marked this conversation as resolved.
}

break;
}
case ISD::FGETSIGN:
// All bits are zero except the low bit.
Known.Zero.setBitsFrom(1);
Expand Down
14 changes: 13 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3080,8 +3080,13 @@ bool TargetLowering::SimplifyDemandedBits(
}
case ISD::FABS: {
SDValue Op0 = Op.getOperand(0);
APInt SignMask = APInt::getSignMask(BitWidth);
EVT SVT = Op0.getValueType();

// The position of the sign bit for ppc_fp128 is endian-dependent.
if (!APFloat::hasSignBitInMSB(SVT.getFltSemantics()) || SVT == MVT::ppcf128)
break;

@folkertdev folkertdev Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be able to do slightly better here, but I'd rather be correct and slightly inefficient for a type that is almost never used than slightly more efficient and potentially subtly wrong.


APInt SignMask = APInt::getSignMask(BitWidth);
if (!DemandedBits.intersects(SignMask))
return TLO.CombineTo(Op, Op0);

Expand Down Expand Up @@ -3133,6 +3138,13 @@ bool TargetLowering::SimplifyDemandedBits(
}
case ISD::FNEG: {
SDValue Op0 = Op.getOperand(0);
EVT SVT = Op0.getValueType();

// The logic below assumes the sign bit is in the MSB.
// ppc_fp128 has two sign bits (at bits 127 and 63).
if (!APFloat::hasSignBitInMSB(SVT.getFltSemantics()) || SVT == MVT::ppcf128)
break;

Comment on lines +3143 to +3147

@folkertdev folkertdev Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another instance

APInt SignMask = APInt::getSignMask(BitWidth);

if (!DemandedBits.intersects(SignMask))
Expand Down
5 changes: 5 additions & 0 deletions llvm/lib/Support/KnownFPClass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,11 @@ KnownBits KnownFPClass::toKnownBits(const fltSemantics &FltSemantics) const {
if (FPClasses == fcNone)
return Known;

// The code below assumes the sign bit is the MSB.
if (!APFloat::hasSignBitInMSB(FltSemantics) ||
&FltSemantics == &APFloat::PPCDoubleDouble())
return Known;

if (isKnownNever(fcNormal | fcSubnormal | fcNan)) {
Known.setAllConflict();

Expand Down
29 changes: 28 additions & 1 deletion llvm/test/CodeGen/PowerPC/copysignl.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,13 @@ entry:
define ppc_fp128 @copysign_signmask(ppc_fp128 %x, i1 %s) {
; LE-LABEL: copysign_signmask:
; LE: # %bb.0: # %entry
; LE-NEXT: fmr 0, 1
; LE-NEXT: xsabsdp 1, 1
; LE-NEXT: xscmpudp 0, 0, 1
; LE-NEXT: beq 0, .LBB6_2
; LE-NEXT: # %bb.1: # %entry
; LE-NEXT: xsnegdp 2, 2
; LE-NEXT: .LBB6_2: # %entry
Comment on lines +386 to +392

@folkertdev folkertdev Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copysign must copy both sign bits

@lei137 lei137 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is that? Shouldn't it be endian based?

@folkertdev folkertdev Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that both sign bits contribute to the sign of the actual value. For instance

#include <stdio.h>
#include <math.h>

union pair { long double ld; double d[2]; };

int main(void)
{
    union pair a = { .ld =   1.0L - 0x1p-60L };
    union pair b = { .ld = -(4.0L - 0x1p-58L) };
    union pair c;

    c.ld = copysignl(a.ld, b.ld);

    printf("a = %g %g = %.36Lg\n", a.d[0], a.d[1], a.ld);
    printf("b = %g %g = %.36Lg\n", b.d[0], b.d[1], b.ld);
    printf("c = %g %g = %.36Lg\n", c.d[0], c.d[1], c.ld);

    union pair wrong1 = { .d = { c.d[0], -c.d[1]} };
    union pair wrong2 = { .d = { -c.d[0], c.d[1]} };
    printf("wrong1 = %g %g = %.36Lg\n", wrong1.d[0], wrong1.d[1], wrong1.ld);
    printf("wrong2 = %g %g = %.36Lg\n", wrong2.d[0], wrong2.d[1], wrong2.ld);

    return 0;
}

With

$ powerpc64le-linux-gnu-gcc ppcf128_copysign.c -o cs
$ qemu-ppc64le -L /usr/powerpc64le-linux-gnu ./cs

Gives

a = 1 -8.67362e-19 = 0.999999999999999999132638262011596453
b = -4 3.46945e-18 = -3.99999999999999999653055304804638581
c = -1 8.67362e-19 = -0.999999999999999999132638262011596453

wrong1 = -1 -8.67362e-19 = -1.00000000000000000086736173798840355
wrong2 = 1 8.67362e-19 = 1.00000000000000000086736173798840355

As a ppcfp128, a is positive, b is negative, and c has the value of a but the sign of b.

The example was chosen so that the corresponding components of a and b have different sign bits. So if you only copied the sign bit of one half, the result would be incorrect: both sign bits from b must be copied for c to have the right value.

The wrong variants, where we copy only one of the sign bits, just have an incorrect value.

@folkertdev folkertdev Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add: my understanding is that valid ppcf128 values are normalized, so that the high component is always larger in magnitude than the low component, and hence to determine the sign you can just read the sign bit of the high component. But to copy the sign, you need to consider both sign bits.

; LE-NEXT: mflr 0
; LE-NEXT: stdu 1, -32(1)
; LE-NEXT: std 0, 48(1)
Expand All @@ -400,6 +407,13 @@ define ppc_fp128 @copysign_signmask(ppc_fp128 %x, i1 %s) {
;
; BE-LABEL: copysign_signmask:
; BE: # %bb.0: # %entry
; BE-NEXT: fmr 0, 1
; BE-NEXT: fabs 1, 1
; BE-NEXT: fcmpu 0, 0, 1
; BE-NEXT: beq 0, .LBB6_2
; BE-NEXT: # %bb.1: # %entry
; BE-NEXT: fneg 2, 2
; BE-NEXT: .LBB6_2: # %entry
; BE-NEXT: mflr 0
; BE-NEXT: stdu 1, -128(1)
; BE-NEXT: std 0, 144(1)
Expand All @@ -419,6 +433,13 @@ define ppc_fp128 @copysign_signmask(ppc_fp128 %x, i1 %s) {
;
; BE-VSX-LABEL: copysign_signmask:
; BE-VSX: # %bb.0: # %entry
; BE-VSX-NEXT: fmr 0, 1
; BE-VSX-NEXT: xsabsdp 1, 1
; BE-VSX-NEXT: xscmpudp 0, 0, 1
; BE-VSX-NEXT: beq 0, .LBB6_2
; BE-VSX-NEXT: # %bb.1: # %entry
; BE-VSX-NEXT: xsnegdp 2, 2
; BE-VSX-NEXT: .LBB6_2: # %entry
; BE-VSX-NEXT: mflr 0
; BE-VSX-NEXT: stdu 1, -128(1)
; BE-VSX-NEXT: std 0, 144(1)
Expand All @@ -442,7 +463,13 @@ define ppc_fp128 @copysign_signmask(ppc_fp128 %x, i1 %s) {
; BE32-NEXT: stw 0, 100(1)
; BE32-NEXT: .cfi_def_cfa_offset 96
; BE32-NEXT: .cfi_offset lr, 4
; BE32-NEXT: stfd 1, 40(1)
; BE32-NEXT: fabs 0, 1
; BE32-NEXT: fcmpu 0, 1, 0
; BE32-NEXT: beq 0, .LBB6_2
; BE32-NEXT: # %bb.1: # %entry
; BE32-NEXT: fneg 2, 2
; BE32-NEXT: .LBB6_2: # %entry
; BE32-NEXT: stfd 0, 40(1)
; BE32-NEXT: slwi 3, 3, 31
; BE32-NEXT: stw 3, 64(1)
; BE32-NEXT: li 4, 0
Expand Down
10 changes: 5 additions & 5 deletions llvm/test/CodeGen/PowerPC/fneg.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,9 @@ entry:
define i1 @fneg_msb(ppc_fp128 nofpclass(nan ninf nsub nnorm nzero) %x) {
; LE-LABEL: fneg_msb:
; LE: # %bb.0: # %entry
; LE-NEXT: li 3, 1
; LE-NEXT: mffprd 3, 2
; LE-NEXT: not 3, 3
; LE-NEXT: rldicl 3, 3, 1, 63
Comment on lines -148 to +150

@folkertdev folkertdev Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit 63 determines the sign

; LE-NEXT: blr
;
; BE-LABEL: fneg_msb:
Expand All @@ -167,8 +169,7 @@ entry:
define i1 @fneg_bit63(ppc_fp128 nofpclass(nan ninf nsub nnorm nzero) %x) {
; LE-LABEL: fneg_bit63:
; LE: # %bb.0: # %entry
; LE-NEXT: mffprd 3, 1
; LE-NEXT: rldicl 3, 3, 1, 63
; LE-NEXT: li 3, 1
; LE-NEXT: blr
;
; BE-LABEL: fneg_bit63:
Expand Down Expand Up @@ -200,8 +201,7 @@ entry:
define i1 @fneg_fneg_msb(ppc_fp128 nofpclass(nan ninf nsub nnorm nzero) %x) {
; LE-LABEL: fneg_fneg_msb:
; LE: # %bb.0: # %entry
; LE-NEXT: mffprd 3, 1
; LE-NEXT: rldicl 3, 3, 1, 63
; LE-NEXT: li 3, 0

@folkertdev folkertdev Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we now statically know the value of bit 63

; LE-NEXT: blr
;
; BE-LABEL: fneg_fneg_msb:
Expand Down
101 changes: 101 additions & 0 deletions llvm/test/CodeGen/PowerPC/fp128-fabs.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s -check-prefix=LE
; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu < %s | FileCheck %s -check-prefix=BE
; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-linux-gnu < %s | FileCheck %s -check-prefix=BE32

; On little-endian powerpc the bit position of the sign bit is bit 63, not
; bit 127. However, APFloat reports that the sign bit is the MSB. Ensure
; that we do not incorrectly optimize based on the (false!) assumption that
; fabs just clears the MSB.

define i1 @msb_set(ppc_fp128 %x) {
; LE-LABEL: msb_set:
; LE: # %bb.0: # %entry
; LE-NEXT: mffprd 3, 1
; LE-NEXT: mffprd 4, 2
; LE-NEXT: xor 3, 4, 3
; LE-NEXT: rldicl 3, 3, 1, 63
; LE-NEXT: blr
;
; BE-LABEL: msb_set:
; BE: # %bb.0: # %entry
; BE-NEXT: li 3, 0
; BE-NEXT: blr
;
; BE32-LABEL: msb_set:
; BE32: # %bb.0: # %entry
; BE32-NEXT: li 3, 0
; BE32-NEXT: blr
entry:
%a = call ppc_fp128 @llvm.fabs.ppcf128(ppc_fp128 %x)
%v = bitcast ppc_fp128 %a to i128
%cmp = icmp slt i128 %v, 0
ret i1 %cmp
}

; On BE the ppc_fp128 sign bit is stored in bit 127, this information
; makes the function return a constant there.
define i1 @fabs_clears_sign_be(ppc_fp128 %x) {
; LE-LABEL: fabs_clears_sign_be:
; LE: # %bb.0: # %entry
; LE-NEXT: mffprd 3, 1
; LE-NEXT: mffprd 4, 2
; LE-NEXT: xor 3, 4, 3
; LE-NEXT: rldicl 3, 3, 1, 63
; LE-NEXT: blr
;
; BE-LABEL: fabs_clears_sign_be:
; BE: # %bb.0: # %entry
; BE-NEXT: li 3, 0
; BE-NEXT: blr
;
; BE32-LABEL: fabs_clears_sign_be:
; BE32: # %bb.0: # %entry
; BE32-NEXT: li 3, 0
; BE32-NEXT: blr
entry:
%neg = fneg ppc_fp128 %x
%a = call ppc_fp128 @llvm.fabs.ppcf128(ppc_fp128 %neg)
%v = bitcast ppc_fp128 %a to i128
%cmp = icmp slt i128 %v, 0
ret i1 %cmp
}

; On LE the ppc_fp128 sign bit is stored in bit 63, this information
; makes the function return a constant there.
define i1 @fabs_clears_sign_le(ppc_fp128 %x) {
; LE-LABEL: fabs_clears_sign_le:
; LE: # %bb.0: # %entry
; LE-NEXT: li 3, 0
; LE-NEXT: blr
;
; BE-LABEL: fabs_clears_sign_le:
; BE: # %bb.0: # %entry
; BE-NEXT: stfd 1, -16(1)
; BE-NEXT: stfd 2, -8(1)
; BE-NEXT: ld 3, -16(1)
; BE-NEXT: ld 4, -8(1)
; BE-NEXT: xor 3, 4, 3
; BE-NEXT: rldicl 3, 3, 1, 63
; BE-NEXT: blr
;
; BE32-LABEL: fabs_clears_sign_le:
; BE32: # %bb.0: # %entry
; BE32-NEXT: stwu 1, -32(1)
; BE32-NEXT: .cfi_def_cfa_offset 32
; BE32-NEXT: stfd 1, 24(1)
; BE32-NEXT: stfd 2, 16(1)
; BE32-NEXT: lwz 3, 24(1)
; BE32-NEXT: lwz 4, 16(1)
; BE32-NEXT: xor 3, 4, 3
; BE32-NEXT: srwi 3, 3, 31
; BE32-NEXT: addi 1, 1, 32
; BE32-NEXT: blr
entry:
%neg = fneg ppc_fp128 %x
%a = call ppc_fp128 @llvm.fabs.ppcf128(ppc_fp128 %neg)
%v = bitcast ppc_fp128 %a to i128
%masked = and i128 %v, 9223372036854775808 ; 1 << 63
%cmp = icmp ne i128 %masked, 0
ret i1 %cmp
}
6 changes: 4 additions & 2 deletions llvm/test/CodeGen/PowerPC/nofpclass.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
define i1 @negative(ppc_fp128 nofpclass(nan pinf psub pnorm pzero) %x) {
; LE-LABEL: negative:
; LE: # %bb.0: # %entry
; LE-NEXT: li 3, 1
; LE-NEXT: mffprd 3, 2
; LE-NEXT: rldicl 3, 3, 1, 63
; LE-NEXT: blr
;
; BE-LABEL: negative:
Expand All @@ -33,7 +34,8 @@ entry:
define i1 @nonnegative(ppc_fp128 nofpclass(nan ninf nsub nnorm nzero) %x) {
; LE-LABEL: nonnegative:
; LE: # %bb.0: # %entry
; LE-NEXT: li 3, 0
; LE-NEXT: mffprd 3, 2
; LE-NEXT: rldicl 3, 3, 1, 63
; LE-NEXT: blr
;
; BE-LABEL: nonnegative:
Expand Down
5 changes: 4 additions & 1 deletion llvm/test/Transforms/InstCombine/known-bits.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,9 +1537,12 @@ define i16 @test_inf_only_bfloat(bfloat nofpclass(nan sub norm zero) %x) {
ret i16 %and
}

; A bitcast from ppc_fp128 to i128 is endian-dependent.
define i128 @test_inf_only_ppc_fp128(ppc_fp128 nofpclass(nan sub norm zero) %x) {
; CHECK-LABEL: @test_inf_only_ppc_fp128(
; CHECK-NEXT: ret i128 9218868437227405312
; CHECK-NEXT: [[TMP1:%.*]] = call ppc_fp128 @llvm.fabs.ppcf128(ppc_fp128 [[X:%.*]])
; CHECK-NEXT: [[AND:%.*]] = bitcast ppc_fp128 [[TMP1]] to i128
; CHECK-NEXT: ret i128 [[AND]]
;
%y = bitcast ppc_fp128 %x to i128
%and = and i128 %y, 170141183460469231731687303715884105727
Expand Down
Loading