[PowerPC] fix `ppc_fp128` `FABS` miscompile by folkertdev · Pull Request #209286 · llvm/llvm-project

folkertdev · 2026-07-13T19:53:55Z

fixes #209034

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. As I understand it, it is actually right about that because in the APFloat internal representation the sign bit is in fact in the MSB, but using this information for runtime values is incorrect for LE.

All three fix locations are required to get the correct output for this reproducer.

I'm assuming there are more ways that this can fail, so a more structural solution would be neat. I don't know how to do that though, so in the meantime we can accumulate test cases and plug the gaps.

folkertdev · 2026-07-13T19:54:55Z

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


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.

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. This causes no end of subtle miscompilations, commit fixes just one. All three fix locations are required to get the correct output for this reproducer.

llvmorg-github-actions · 2026-07-13T23:03:06Z

@llvm/pr-subscribers-backend-powerpc
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-llvm-transforms

Author: Folkert de Vries (folkertdev)

Changes

fixes #209034

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. As I understand it, it is actually right about that because in the APFloat internal representation the sign bit is in fact in the MSB, but using this information for runtime values is incorrect for LE.

All three fix locations are required to get the correct output for this reproducer.

I'm assuming there are more ways that this can fail, so a more structural solution would be neat. I don't know how to do that though, so in the meantime we can accumulate test cases and plug the gaps.

Full diff: https://github.com/llvm/llvm-project/pull/209286.diff

5 Files Affected:

(modified) llvm/lib/Analysis/ValueTracking.cpp (+5)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+15-2)
(modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+6-1)
(added) llvm/test/CodeGen/PowerPC/fp128-fabs.ll (+36)
(modified) llvm/test/Transforms/InstCombine/known-bits.ll (+4-1)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 59631873305d4..fd1a4b5b4d401 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1528,6 +1528,11 @@ static void computeKnownBitsFromOperator(const Operator *I,
           computeKnownFPClass(V, DemandedElts, fcAllFlags, Q, Depth + 1);
       FPClassTest FPClasses = Result.KnownFPClasses;
 
+      // The position of the sign bit for ppc_fp128 is endian-dependent.
+      if (!APFloat::hasSignBitInMSB(FPType->getFltSemantics()) ||
+          FPType->isPPC_FP128Ty())
+        break;
+
       // TODO: Treat it as zero/poison if the use of I is unreachable.
       if (FPClasses == fcNone)
         break;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 626803ed92a40..a6306f9f57747 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -4157,11 +4157,24 @@ KnownBits SelectionDAG::computeKnownBits(SDValue Op, const APInt &DemandedElts,
 
     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();
+    }
+
     break;
+  }
   case ISD::FGETSIGN:
     // All bits are zero except the low bit.
     Known.Zero.setBitsFrom(1);
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index bca34c5c347ee..c25e12d45c2ed 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -3058,8 +3058,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;
 
+    APInt SignMask = APInt::getSignMask(BitWidth);
     if (!DemandedBits.intersects(SignMask))
       return TLO.CombineTo(Op, Op0);
 
diff --git a/llvm/test/CodeGen/PowerPC/fp128-fabs.ll b/llvm/test/CodeGen/PowerPC/fp128-fabs.ll
new file mode 100644
index 0000000000000..ef4932c70deb7
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/fp128-fabs.ll
@@ -0,0 +1,36 @@
+; 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
+}
+
+declare ppc_fp128 @llvm.fabs.ppcf128(ppc_fp128)
diff --git a/llvm/test/Transforms/InstCombine/known-bits.ll b/llvm/test/Transforms/InstCombine/known-bits.ll
index acf09bc03c1eb..30eb431644397 100644
--- a/llvm/test/Transforms/InstCombine/known-bits.ll
+++ b/llvm/test/Transforms/InstCombine/known-bits.ll
@@ -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

lei137

Wondering if this can just be fixed in Knownfpclass::fabs(). Pass the semantics to the function call and handle setting the SignBit there instead?

folkertdev

Wondering if this can just be fixed in Knownfpclass::fabs(). Pass the semantics to the function call and handle setting the SignBit there instead?

I don't think so, this bug is all over the place.

github-actions · 2026-07-31T19:48:38Z

✅ With the latest revision this PR passed the C/C++ code formatter.

folkertdev · 2026-07-31T20:15:39Z

+    // 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;
+


another instance

folkertdev · 2026-07-31T20:16:05Z

+; 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


copysign must copy both sign bits

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

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.

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.

folkertdev · 2026-07-31T20:17:08Z

-; LE-NEXT:    li 3, 1
+; LE-NEXT:    mffprd 3, 2
+; LE-NEXT:    not 3, 3
+; LE-NEXT:    rldicl 3, 3, 1, 63


bit 63 determines the sign

folkertdev · 2026-07-31T20:17:45Z

 ; LE:       # %bb.0: # %entry
-; LE-NEXT:    mffprd 3, 1
-; LE-NEXT:    rldicl 3, 3, 1, 63
+; LE-NEXT:    li 3, 0


here we now statically know the value of bit 63

folkertdev commented Jul 13, 2026

View reviewed changes

folkertdev force-pushed the fix-ppc_f128-fabs-bug branch from c5bcfca to 2d5d6e1 Compare July 13, 2026 20:35

folkertdev changed the title ~~[PowerPC] fix ppc_fp128 FABS bug~~ [PowerPC] fix ppc_fp128 FABS miscompile Jul 13, 2026

folkertdev marked this pull request as ready for review July 13, 2026 23:02

folkertdev requested a review from nikic as a code owner July 13, 2026 23:02

llvmorg-github-actions Bot added backend:PowerPC llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:SelectionDAG SelectionDAGISel as well llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Jul 13, 2026

RKSimon requested review from amy-kwan and lei137 July 14, 2026 08:34

folkertdev mentioned this pull request Jul 14, 2026

C interop: f80, f128 and c_longdouble rust-lang/goals#701

Open

lei137 reviewed Jul 29, 2026

View reviewed changes

Comment thread llvm/lib/Analysis/ValueTracking.cpp Outdated

Comment thread llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Comment thread llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Comment thread llvm/test/CodeGen/PowerPC/fp128-fabs.ll

folkertdev added 3 commits July 30, 2026 11:04

nits and add test

74db7a4

fix endiannes bug in AssertNoFPClass too

d6d6fb5

check that sign knowledge is used on LE too

3070f84

folkertdev commented Jul 30, 2026

View reviewed changes

Comment thread llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Comment thread llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Comment thread llvm/test/CodeGen/PowerPC/fp128-fabs.ll

folkertdev requested a review from lei137 July 30, 2026 22:25

folkertdev added 5 commits July 31, 2026 14:30

fix copysign for ppc_fp128

fa75252

Merge remote-tracking branch 'upstream/main' into fix-ppc_f128-fabs-bug

5c894a9

Merge remote-tracking branch 'upstream/main' into fix-ppc_f128-fabs-bug

f75acfd

update copysign tests

d7b6b74

fix another ppcf128 bug in fneg

42736cc

formatting

efe0d3d

folkertdev commented Jul 31, 2026

View reviewed changes

folkertdev mentioned this pull request Aug 10, 2026

Complex layout phase 2 rust-lang/rust#159981

Draft

Merge remote-tracking branch 'upstream/main' into fix-ppc_f128-fabs-bug

330b268

Uh oh!

Conversation

folkertdev commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvmorg-github-actions Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lei137 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

folkertdev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

llvmorg-github-actions Bot commented Jul 13, 2026 •

edited

Loading

github-actions Bot commented Jul 31, 2026 •

edited

Loading