InstCombine: Recognize fneg when performed as bitcasted integer

[arsenm]

Summary

This is a resurrection of D18874. This was previously wrong with
fneg conflated with fsub, but we now have a proper fneg instruction.
Additionally, I think it is now clearer that IR float=IEEE float,
and a different bit layout would require adding a different IR type.

[efriedma]

In general, we need to be careful introducing floating-point values where the don't otherwise exist, for a couple reasons. One, loading a value into an x87 floating-point register can quiet SNaNs. And two, certain compiler options obligate us to avoid floating-point code. That said, if the source value is already floating-point, it should be fine to introduce floating-point operations on it, I think.

The other possible issue is the interaction with "UnsafeFPMath" and related CodeGen options. Fast-math is supposed to be a floating-point thing, but if we transform integer ops into floating-point ops, they effectively apply to integer math. Which is probably (?) not what the user expects. (I think fast-math flags on individual instructions should interact correctly, since the "fneg" created by this transform won't have any flags; my concern is with the global flags.)

[arsenm]

In D151934#4388968, @efriedma wrote:

In general, we need to be careful introducing floating-point values where the don't otherwise exist, for a couple reasons. One, loading a value into an x87 floating-point register can quiet SNaNs. And two, certain compiler options obligate us to avoid floating-point code. That said, if the source value is already floating-point, it should be fine to introduce floating-point operations on it, I think.

fneg is defined as a bitwise operation. It's not a real FP operation, it's a bitwise operation on FP types. It's only permitted to change the sign bit. If x87 decided to quiet an snan in the fneg implementation, the lowering is broken. All concerns about floating-pointness and fast math should be gone since fsub is no longer involved.

[efriedma]

If you compile the following for x86 with -mno-sse:

void f(float *x) { *x = -*x; }

You get the following:

movl    4(%esp), %eax
flds    (%eax)
fchs
fstps   (%eax)
retl

This transforms snan into qnan. That's arguably incorrect, but it's what every x86 compiler has emitted since forever, so nobody has tried to fix it.

The whole thing isn't really relevant for this patch, though, because you trigger the exact same thing if you bitcast the float to an i32. (The exception is triggered on the flds which loads the register, not the fchs which actually flips the sign bit.)

---

In any case, I'm more concerned about the UnsafeFPMath thing.

[arsenm]

In D151934#4389126, @efriedma wrote:

In any case, I'm more concerned about the UnsafeFPMath thing.

We already do this transform in the DAG, guarded by isFNegFree. I don't think this is a real issue. You can't really do anything unsafe with an fneg on its own, it has to be connected to something where the unsafe flag already applied. e.g. fadd (x, fneg(x)) -> 0 already had to have an nnan

[efriedma]

You can't really do anything unsafe with an fneg on its own, it has to be connected to something where the unsafe flag already applied.

If the fneg is the only use of an unsafe floating-point operation, there isn't really any way to observe whether the fneg itself is safe or unsafe. But we could produce contradictory results if the operand has multiple uses. For example, it's normally safe to assume x ^ 0x80000000 != x. But if fneg is treated as an unsafe math operation, there are some situations where fneg(x) == x. This could lead to a miscompile, at least in theory. Not sure how likely it is for anyone to trip over that in practice.

[arsenm]

Check noimplicitfloat (though I don't think it should be necessary, but it isn't really well defined what it means)

[arsenm]

In D151934#4411516, @efriedma wrote:

You can't really do anything unsafe with an fneg on its own, it has to be connected to something where the unsafe flag already applied.

If the fneg is the only use of an unsafe floating-point operation, there isn't really any way to observe whether the fneg itself is safe or unsafe. But we could produce contradictory results if the operand has multiple uses. For example, it's normally safe to assume x ^ 0x80000000 != x. But if fneg is treated as an unsafe math operation, there are some situations where fneg(x) == x. This could lead to a miscompile, at least in theory. Not sure how likely it is for anyone to trip over that in practice.

I think you have no right to complain once your code touches fast math code. It's not a miscompile if you permitted it. The sign bit changing would have to originate from the fast math imbued instruction

[goldstein.w.n]

imo should require a cast back to float. If we are keeping it as an integer
I think xor is more recognizable/canonical.

[efriedma]

LGTM

I think you have no right to complain once your code touches fast math code. It's not a miscompile if you permitted it. The sign bit changing would have to originate from the fast math imbued instruction

The point here is that there should be a boundary between fast and non-fast instructions: if you have a "fast" algorithm, you should be able to use its output in non-"fast" code without infecting it.

But I think it only ends up being an issue with the function-wide flag, not the per-instruction flags, so anyone who cares about that can just avoid setting that bit.

imo should require a cast back to float. If we are keeping it as an integer, I think xor is more recognizable/canonical.

fneg seems more likely to actually correspond to a natively supported instruction, and more likely to be combined with adjacent operations.

[goldstein.w.n]

In D151934#4632720, @efriedma wrote:

LGTM

I think you have no right to complain once your code touches fast math code. It's not a miscompile if you permitted it. The sign bit changing would have to originate from the fast math imbued instruction

The point here is that there should be a boundary between fast and non-fast instructions: if you have a "fast" algorithm, you should be able to use its output in non-"fast" code without infecting it.

But I think it only ends up being an issue with the function-wide flag, not the per-instruction flags, so anyone who cares about that can just avoid setting that bit.

imo should require a cast back to float. If we are keeping it as an integer, I think xor is more recognizable/canonical.

fneg seems more likely to actually correspond to a natively supported instruction, and more likely to be combined with adjacent operations.

Depends which context you are talking about. If its with other float instructions sure. If its with integer instructions I'm not so sure.
Thats why I think it should be (bitcast (xor (bitcast)) not just (xor (bitcast)).

Edit: Although it could go either way, so no real strong opposition.

[arsenm]

In D151934#4632790, @goldstein.w.n wrote:

Depends which context you are talking about. If its with other float instructions sure. If its with integer instructions I'm not so sure.
Thats why I think it should be (bitcast (xor (bitcast)) not just (xor (bitcast)).

Edit: Although it could go either way, so no real strong opposition.

Canonical form tends to push things into sources, not the other direction. If the integer operation was useful downstream it should have folded before reaching here

[arsenm]

50a9b3d8a5220c21c57197ef4c9483a2b9d14fd5

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

if (match(Op1, m_APInt(RHSC))) {
           *RHSC == APInt::getAllOnes(Ty->getScalarSizeInBits()).lshr(*C)) {
         Value *NotX = Builder.CreateNot(X);
         return BinaryOperator::CreateLShr(NotX, ConstantInt::get(Ty, *C));
       }
       // TODO: We could handle 'ashr' here as well. That would be matching
       //       a 'not' op and moving it before the shift. Doing that requires
       //       preventing the inverse fold in canShiftBinOpWithConstantRHS().
     }
+
+    // If we are XORing the sign bit of a floating-point value, convert
+    // this to fneg, then cast back to integer.
+    //
+    // Assumes any IEEE-represented type has the sign bit in the high bit.
+    // TODO: Unify with APInt matcher. This version allows undef unlike m_APInt
+    Value *CastOp;
+    if (match(Op0, m_BitCast(m_Value(CastOp))) && match(Op1, m_SignMask()) &&
+        !Builder.GetInsertBlock()->getParent()->hasFnAttribute(
+            Attribute::NoImplicitFloat)) {
+      Type *EltTy = CastOp->getType()->getScalarType();
+      if (EltTy->isFloatingPointTy() && EltTy->isIEEE() &&
+          EltTy->getPrimitiveSizeInBits() ==
+          I.getType()->getScalarType()->getPrimitiveSizeInBits()) {
+        Value *FNeg = Builder.CreateFNeg(CastOp);
+        return new BitCastInst(FNeg, I.getType());
+      }
+    }
   }
 
   // FIXME: This should not be limited to scalar (pull into APInt match above).
   {
     Value *X;
     ConstantInt *C1, *C2, *C3;
     // ((X^C1) >> C2) ^ C3 -> (X>>C2) ^ ((C1>>C2)^C3)
     if (match(Op1, m_ConstantInt(C3)) &&

llvm/test/Transforms/InstCombine/fneg-as-int.ll

;
   %bc = bitcast <2 x float> %x to <2 x i32>
   %xor = xor <2 x i32> %bc, <i32 -2147483648, i32 -2147483648>
   ret <2 x i32> %xor
 }
 
 define float @fneg_as_int_f32_castback(float %val) {
 ; CHECK-LABEL: define float @fneg_as_int_f32_castback
 ; CHECK-SAME: (float [[VAL:%.*]]) {
-; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast float [[VAL]] to i32
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[BITCAST]], -2147483648
-; CHECK-NEXT:    [[FNEG:%.*]] = bitcast i32 [[XOR]] to float
-; CHECK-NEXT:    ret float [[FNEG]]
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg float [[VAL]]
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %bitcast = bitcast float %val to i32
   %xor = xor i32 %bitcast, -2147483648
   %fneg = bitcast i32 %xor to float
   ret float %fneg
 }
 
 define float @not_fneg_as_int_f32_castback_wrongconst(float %val) {
 ; CHECK-LABEL: define float @not_fneg_as_int_f32_castback_wrongconst
 ; CHECK-SAME: (float [[VAL:%.*]]) {
 ; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast float [[VAL]] to i32
 ; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[BITCAST]], -2147483647
 ; CHECK-NEXT:    [[FNEG:%.*]] = bitcast i32 [[XOR]] to float
 ; CHECK-NEXT:    ret float [[FNEG]]
 ;
   %bitcast = bitcast float %val to i32
   %xor = xor i32 %bitcast, -2147483647
   %fneg = bitcast i32 %xor to float
   ret float %fneg
 }
 
 define float @fneg_as_int_f32_castback_multi_use(float %val, ptr %ptr) {
 ; CHECK-LABEL: define float @fneg_as_int_f32_castback_multi_use
 ; CHECK-SAME: (float [[VAL:%.*]], ptr [[PTR:%.*]]) {
-; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast float [[VAL]] to i32
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[BITCAST]], -2147483648
-; CHECK-NEXT:    store i32 [[XOR]], ptr [[PTR]], align 4
-; CHECK-NEXT:    [[FNEG:%.*]] = bitcast i32 [[XOR]] to float
-; CHECK-NEXT:    ret float [[FNEG]]
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg float [[VAL]]
+; CHECK-NEXT:    store float [[TMP1]], ptr [[PTR]], align 4
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %bitcast = bitcast float %val to i32
   %xor = xor i32 %bitcast, -2147483648
   store i32 %xor, ptr %ptr
   %fneg = bitcast i32 %xor to float
   ret float %fneg
 }
 
 define i64 @fneg_as_int_f64(double %x) {
 ; CHECK-LABEL: define i64 @fneg_as_int_f64
 ; CHECK-SAME: (double [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast double [[X]] to i64
-; CHECK-NEXT:    [[XOR:%.*]] = xor i64 [[BC]], -9223372036854775808
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast double [[TMP1]] to i64
 ; CHECK-NEXT:    ret i64 [[XOR]]
 ;
   %bc = bitcast double %x to i64
   %xor = xor i64 %bc, -9223372036854775808
   ret i64 %xor
 }
 
 define <2 x i64> @fneg_as_int_v2f64(<2 x double> %x) {
 ; CHECK-LABEL: define <2 x i64> @fneg_as_int_v2f64
 ; CHECK-SAME: (<2 x double> [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast <2 x double> [[X]] to <2 x i64>
-; CHECK-NEXT:    [[XOR:%.*]] = xor <2 x i64> [[BC]], <i64 -9223372036854775808, i64 -9223372036854775808>
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg <2 x double> [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast <2 x double> [[TMP1]] to <2 x i64>
 ; CHECK-NEXT:    ret <2 x i64> [[XOR]]
 ;
   %bc = bitcast <2 x double> %x to <2 x i64>
   %xor = xor <2 x i64> %bc, <i64 -9223372036854775808, i64 -9223372036854775808>
   ret <2 x i64> %xor
 }
 
 define i64 @fneg_as_int_f64_swap(double %x) {
 ; CHECK-LABEL: define i64 @fneg_as_int_f64_swap
 ; CHECK-SAME: (double [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast double [[X]] to i64
-; CHECK-NEXT:    [[XOR:%.*]] = xor i64 [[BC]], -9223372036854775808
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast double [[TMP1]] to i64
 ; CHECK-NEXT:    ret i64 [[XOR]]
 ;
   %bc = bitcast double %x to i64
   %xor = xor i64 -9223372036854775808, %bc
   ret i64 %xor
 }
 
 define i32 @fneg_as_int_f32(float %x) {
 ; CHECK-LABEL: define i32 @fneg_as_int_f32
 ; CHECK-SAME: (float [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast float [[X]] to i32
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[BC]], -2147483648
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg float [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast float [[TMP1]] to i32
 ; CHECK-NEXT:    ret i32 [[XOR]]
 ;
   %bc = bitcast float %x to i32
   %xor = xor i32 %bc, -2147483648
   ret i32 %xor
 }
 
 define <2 x i32> @fneg_as_int_v2f32(<2 x float> %x) {
 ; CHECK-LABEL: define <2 x i32> @fneg_as_int_v2f32
 ; CHECK-SAME: (<2 x float> [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast <2 x float> [[X]] to <2 x i32>
-; CHECK-NEXT:    [[XOR:%.*]] = xor <2 x i32> [[BC]], <i32 -2147483648, i32 -2147483648>
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg <2 x float> [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast <2 x float> [[TMP1]] to <2 x i32>
 ; CHECK-NEXT:    ret <2 x i32> [[XOR]]
 ;
   %bc = bitcast <2 x float> %x to <2 x i32>
   %xor = xor <2 x i32> %bc, <i32 -2147483648, i32 -2147483648>
   ret <2 x i32> %xor
 }
 
 define <2 x i32> @not_fneg_as_int_v2f32_nonsplat(<2 x float> %x) {
 ; CHECK-LABEL: define <2 x i32> @not_fneg_as_int_v2f32_nonsplat
 ; CHECK-SAME: (<2 x float> [[X:%.*]]) {
 ; CHECK-NEXT:    [[BC:%.*]] = bitcast <2 x float> [[X]] to <2 x i32>
 ; CHECK-NEXT:    [[XOR:%.*]] = xor <2 x i32> [[BC]], <i32 -2147483648, i32 -2147483647>
 ; CHECK-NEXT:    ret <2 x i32> [[XOR]]
 ;
   %bc = bitcast <2 x float> %x to <2 x i32>
   %xor = xor <2 x i32> %bc, <i32 -2147483648, i32 -2147483647>
   ret <2 x i32> %xor
 }
 
 define <3 x i32> @fneg_as_int_v3f32_undef(<3 x float> %x) {
 ; CHECK-LABEL: define <3 x i32> @fneg_as_int_v3f32_undef
 ; CHECK-SAME: (<3 x float> [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast <3 x float> [[X]] to <3 x i32>
-; CHECK-NEXT:    [[XOR:%.*]] = xor <3 x i32> [[BC]], <i32 -2147483648, i32 undef, i32 -2147483648>
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg <3 x float> [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast <3 x float> [[TMP1]] to <3 x i32>
 ; CHECK-NEXT:    ret <3 x i32> [[XOR]]
 ;
   %bc = bitcast <3 x float> %x to <3 x i32>
   %xor = xor <3 x i32> %bc, <i32 -2147483648, i32 undef, i32 -2147483648>
   ret <3 x i32> %xor
 }
 
 ; Make sure that only a bitcast is transformed.
;
   %bc = bitcast fp128 %x to i128
   %xor = xor i128 %bc, -9223372036854775808
   ret i128 %xor
 }
 
 define i128 @fneg_as_int_fp128_f128_mask(fp128 %x) {
 ; CHECK-LABEL: define i128 @fneg_as_int_fp128_f128_mask
 ; CHECK-SAME: (fp128 [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast fp128 [[X]] to i128
-; CHECK-NEXT:    [[XOR:%.*]] = xor i128 [[BC]], -170141183460469231731687303715884105728
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg fp128 [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast fp128 [[TMP1]] to i128
 ; CHECK-NEXT:    ret i128 [[XOR]]
 ;
   %bc = bitcast fp128 %x to i128
   %xor = xor i128 %bc, -170141183460469231731687303715884105728
   ret i128 %xor
 }
 
 define i16 @fneg_as_int_f16(half %x) {
 ; CHECK-LABEL: define i16 @fneg_as_int_f16
 ; CHECK-SAME: (half [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast half [[X]] to i16
-; CHECK-NEXT:    [[XOR:%.*]] = xor i16 [[BC]], -32768
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg half [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast half [[TMP1]] to i16
 ; CHECK-NEXT:    ret i16 [[XOR]]
 ;
   %bc = bitcast half %x to i16
   %xor = xor i16 %bc, -32768
   ret i16 %xor
 }
 
 define <2 x i16> @fneg_as_int_v2f16(<2 x half> %x) {
 ; CHECK-LABEL: define <2 x i16> @fneg_as_int_v2f16
 ; CHECK-SAME: (<2 x half> [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast <2 x half> [[X]] to <2 x i16>
-; CHECK-NEXT:    [[XOR:%.*]] = xor <2 x i16> [[BC]], <i16 -32768, i16 -32768>
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg <2 x half> [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast <2 x half> [[TMP1]] to <2 x i16>
 ; CHECK-NEXT:    ret <2 x i16> [[XOR]]
 ;
   %bc = bitcast <2 x half> %x to <2 x i16>
   %xor = xor <2 x i16> %bc, <i16 -32768, i16 -32768>
   ret <2 x i16> %xor
 }
 
 define i16 @fneg_as_int_bf16(bfloat %x) {
 ; CHECK-LABEL: define i16 @fneg_as_int_bf16
 ; CHECK-SAME: (bfloat [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast bfloat [[X]] to i16
-; CHECK-NEXT:    [[XOR:%.*]] = xor i16 [[BC]], -32768
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg bfloat [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast bfloat [[TMP1]] to i16
 ; CHECK-NEXT:    ret i16 [[XOR]]
 ;
   %bc = bitcast bfloat %x to i16
   %xor = xor i16 %bc, -32768
   ret i16 %xor
 }
 
 define <2 x i16> @fneg_as_int_v2bf16(<2 x bfloat> %x) {
 ; CHECK-LABEL: define <2 x i16> @fneg_as_int_v2bf16
 ; CHECK-SAME: (<2 x bfloat> [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast <2 x bfloat> [[X]] to <2 x i16>
-; CHECK-NEXT:    [[XOR:%.*]] = xor <2 x i16> [[BC]], <i16 -32768, i16 -32768>
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg <2 x bfloat> [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast <2 x bfloat> [[TMP1]] to <2 x i16>
 ; CHECK-NEXT:    ret <2 x i16> [[XOR]]
 ;
   %bc = bitcast <2 x bfloat> %x to <2 x i16>
   %xor = xor <2 x i16> %bc, <i16 -32768, i16 -32768>
   ret <2 x i16> %xor
 }
 
 define i80 @fneg_as_int_x86_fp80_f64_mask(x86_fp80 %x) {
 ; CHECK-LABEL: define i80 @fneg_as_int_x86_fp80_f64_mask
 ; CHECK-SAME: (x86_fp80 [[X:%.*]]) {
-; CHECK-NEXT:    [[BC:%.*]] = bitcast x86_fp80 [[X]] to i80
-; CHECK-NEXT:    [[XOR:%.*]] = xor i80 [[BC]], -604462909807314587353088
+; CHECK-NEXT:    [[TMP1:%.*]] = fneg x86_fp80 [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = bitcast x86_fp80 [[TMP1]] to i80
 ; CHECK-NEXT:    ret i80 [[XOR]]
 ;
   %bc = bitcast x86_fp80 %x to i80
   %xor = xor i80 %bc, -604462909807314587353088
   ret i80 %xor
 }
 
 define i128 @fneg_as_int_ppc_fp128_f64_mask(ppc_fp128 %x) {