Disable shrinking of SNaN constants

[colpell]

Summary

When expanding FP constants, we attempt to shrink doubles to floats and perform an extending load.
However, on SystemZ, and possibly on other targets (I've only confirmed the problem on SystemZ), the FP extending load instruction may convert SNaN into QNaN, or may cause an exception. So in the general case, we would still like to shrink FP constants, but SNaNs should be left as doubles.

[uweigand]

I agree this seems broken; we shouldn't shrink SNaN constants. However, I still have two comments:

1.) Does this actually have to be platform-specific? I mean, is there any platform where a SNaN constant *can* be stored in converted form? I would have expected that the type-conversion operation needed to load a "shrunk" SNaN constant would be required to actually trigger the "signaling" part of the SNaN (i.e. raise an invalid operation exception) on all platforms ...

2.) To detect an SNaN, you're testing for bitwise-equality against one specific SNaN value. However, in general there can be many different SNaN values. I think you should just test for APF.isSignaling().

[colpell]

Use APFloat::isSignaling instead of bit comparison with SNaN value.

[colpell]

In D22685#494797, @uweigand wrote:

1.) Does this actually have to be platform-specific? I mean, is there any platform where a SNaN constant *can* be stored in converted form? I would have expected that the type-conversion operation needed to load a "shrunk" SNaN constant would be required to actually trigger the "signaling" part of the SNaN (i.e. raise an invalid operation exception) on all platforms ...

This was my initial thought too. It doesn't seem to be a problem on x86 (we get the right value printed and no exceptions), which suggests that it's being handled somehow. I'm not sure about other platforms however.
I left this fix as platform-specific to reduce the possible impact, but if there are no expected drawbacks from disabling it globally, then I'm willing to do so.

2.) To detect an SNaN, you're testing for bitwise-equality against one specific SNaN value. However, in general there can be many different SNaN values. I think you should just test for APF.isSignaling().

Thanks, I've made this change.

[uweigand]

In D22685#495110, @colpell wrote:

In D22685#494797, @uweigand wrote:

1.) Does this actually have to be platform-specific? I mean, is there any platform where a SNaN constant *can* be stored in converted form? I would have expected that the type-conversion operation needed to load a "shrunk" SNaN constant would be required to actually trigger the "signaling" part of the SNaN (i.e. raise an invalid operation exception) on all platforms ...

This was my initial thought too. It doesn't seem to be a problem on x86 (we get the right value printed and no exceptions), which suggests that it's being handled somehow. I'm not sure about other platforms however.
I left this fix as platform-specific to reduce the possible impact, but if there are no expected drawbacks from disabling it globally, then I'm willing to do so.

Hmm, I've had a bit of a closer look into x86 here. First of all, it seems we're only "shrinking" constants in the x86 extended format. And for this format, there's special code in APFloat::convert that marks any conversion of an x86 extended format SNaN to double as "inexact", and therefore the generic shrinking code doesn't do it (because isValueValidForType returns false).

The current code just has a somewhat cryptic comment:

// x86 has some unusual NaNs which cannot be represented in any other
// format; note them here.

but the original code as checked in here:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20081006/068140.html
explicitly says:

// We mark this as Inexact if we are losing information.  This happens
// if are shifting out something other than 0s, or if the x87 long
// double input did not have its integer bit set (pseudo-NaN), or if the
// x87 long double input did not have its QNan bit set (because the x87
// hardware sets this bit when converting a lower-precision NaN to
// x87 long double).

The remark in parenthesis seems to match exactly the problem on System z.

I actually haven't seen any case where LLVM would in fact generate loads of a "shrunk" SNaN constant. Do you have any such case?

B.t.w. the original patch had a test case which seems to have gotten lost in this iteration :-)

[colpell]

Re-add test case that went missing.

[colpell]

Hmm, that definitely explains why x86 isn't running into this issue.
In that case, is the general approach of not shrinking SNaNs in the first place better, or would it make sense to add more special code to APFloat to avoid "inexact" conversions on SystemZ as well?
My feeling is that more backend-specific in APFloat is a bit messy, though it has the potential to fix other cases where we're converting a SNaN (not that I'm currently aware of any).

Here's some example code that results in a "shrunk" SNaN load:

#include <iostream>
#include <limits>

int main()
{
   double snan = std::numeric_limits<double>::signaling_NaN();
   long long *p = (long long *)(&snan);
   std::cout << std::hex << *p << '\n';
   return 0;
}

This will print 7ffc000000000000 with trunk Clang, but will print 7ff4000000000000 with GCC or with my patch applied.

[uweigand]

In D22685#496030, @colpell wrote:

Hmm, that definitely explains why x86 isn't running into this issue.
In that case, is the general approach of not shrinking SNaNs in the first place better, or would it make sense to add more special code to APFloat to avoid "inexact" conversions on SystemZ as well?
My feeling is that more backend-specific in APFloat is a bit messy, though it has the potential to fix other cases where we're converting a SNaN (not that I'm currently aware of any).

Thinking about that, I'm not sure whether the APFloat.convert behavior is really correct. In many cases, it is possible to convert a SNaN in to another SNaN in a different format exactly. (Not all cases, of course. Also, the x86 extended format "pseudo-NaNs" are really special, and can never be converted exactly.)

But simply never doing constant shrinking of a signaling NaN seems the best way to me right now ...

Here's some example code that results in a "shrunk" SNaN load:

[snip]

This will print 7ffc000000000000 with trunk Clang, but will print 7ff4000000000000 with GCC or with my patch applied.

Well, I can reproduce thie 7ffc... result, but only with -m32 and with no optimization. But under those circumstances, I see it with both clang and gcc.

What happens here is that the (non-inlined) call to signaling_NaN() returns an SNaN in the 80-bit extended format in %fp(0) according to the 32-bit floating-point ABI. Code then stores this value to a 64-bit stack slot using FSTPL. *This* instruction is what converts the extended SNaN into a double-precision QNaN (and also sets the inexact exception flag). But that's completely separate issue from constant shrinking ...

[colpell]

Ah, sorry, I should have been more clear -- that code snippet causes the SNaN to be shrunk and converted to a QNaN specifically on SystemZ. I don't have any code that reproduces the issue on other platforms (on x86, the APFloat behaviour is presumably preventing the shrinking from happening).

[uweigand]

In D22685#496061, @uweigand wrote:

In D22685#496030, @colpell wrote:

Hmm, that definitely explains why x86 isn't running into this issue.
In that case, is the general approach of not shrinking SNaNs in the first place better, or would it make sense to add more special code to APFloat to avoid "inexact" conversions on SystemZ as well?
My feeling is that more backend-specific in APFloat is a bit messy, though it has the potential to fix other cases where we're converting a SNaN (not that I'm currently aware of any).

Thinking about that, I'm not sure whether the APFloat.convert behavior is really correct. In many cases, it is possible to convert a SNaN in to another SNaN in a different format exactly. (Not all cases, of course. Also, the x86 extended format "pseudo-NaNs" are really special, and can never be converted exactly.)

To be more specific, I was trying to say the APFloat.convert behavior for non-x86 architectures seems correct, so we should't be adding any SNaN-specific code there. (I think the behavior on *x86* may not be fully correct for non-pseudo SNaNs in the extended format, but that's a different story ...)

But simply never doing constant shrinking of a signaling NaN seems the best way to me right now ...

That still seems the right fix to me: simply never shrink SNaN values on any platform.

Since this is then a cross-platform change, you might want to add some floating-point experts to the approvers list.

In D22685#496087, @colpell wrote:

Ah, sorry, I should have been more clear -- that code snippet causes the SNaN to be shrunk and converted to a QNaN specifically on SystemZ. I don't have any code that reproduces the issue on other platforms (on x86, the APFloat behaviour is presumably preventing the shrinking from happening).

OK, understood.

[colpell]

Update to make this change apply to all platforms rather than just SystemZ.

[colpell]

Re-add SystemZ test case again (sorry!)

[uweigand]

This now looks good to me.

Maybe one minor request: Please add a comment before the if (!APF.isSignaling()) check explaining why this is necessary. Otherwise, I think you can go ahead and check this in now.

[colpell]

Add comment explaining check for !APF.isSignaling()

[uweigand]

LGTM, thanks!

[colpell]

A build bot appears to be failing the test case:
http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/27427

It looks like shrinking is somehow still happening, so presumably APFloat::isSignaling is giving a false negative. I'm not sure what would be causing it to happen on that one specific bot though.

[chapuni]

The builder is configured as -m32.
I guess it would be x87fp-related issue.

On i686 host, NaN might not be preserved.

[chapuni]

The test should pass on x87 after r277813.

[colpell]

I re-added the test and it looks like it hasn't run into any issues overnight. Thanks!

include/llvm/Target/TargetLowering.h

unsigned getNumRegisters(LLVMContext &Context, EVT VT) const {
     llvm_unreachable("Unsupported extended type!");
   }
 
   /// If true, then instruction selection should seek to shrink the FP constant
   /// of the specified type to a smaller type in order to save space and / or
   /// reduce runtime.
   virtual bool ShouldShrinkFPConstant(EVT) const { return true; }
 
+  /// If true (and ShouldShrinkFPConstant is also true), then instruction
+  /// selection should shrink SNaN constants. This is needeed for architectures
+  /// (e.g. SystemZ) in which FP extending loads of a SNaN result in a QNaN
+  /// instead.
+  virtual bool ShouldShrinkSNaNConstant() const { return true; }
+
   // Return true if it is profitable to reduce the given load node to a smaller
   // type.
   //
   // e.g. (i16 (trunc (i32 (load x))) -> i16 load x should be performed
   virtual bool shouldReduceLoadWidth(SDNode *Load,
                                      ISD::LoadExtType ExtTy,
                                      EVT NewVT) const {
     return true;

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

SelectionDAGLegalize::ExpandConstantFP(ConstantFPSDNode *CFP, bool UseCP) {
   EVT VT = CFP->getValueType(0);
   ConstantFP *LLVMC = const_cast<ConstantFP*>(CFP->getConstantFPValue());
   if (!UseCP) {
     assert((VT == MVT::f64 || VT == MVT::f32) && "Invalid type expansion");
     return DAG.getConstant(LLVMC->getValueAPF().bitcastToAPInt(), dl,
                            (VT == MVT::f64) ? MVT::i64 : MVT::i32);
   }
 
+  APFloat APF = CFP->getValueAPF();
   EVT OrigVT = VT;
   EVT SVT = VT;
+
+  if (TLI.ShouldShrinkSNaNConstant() || !APF.isSignaling()) {
     while (SVT != MVT::f32 && SVT != MVT::f16) {
       SVT = (MVT::SimpleValueType)(SVT.getSimpleVT().SimpleTy - 1);
-    if (ConstantFPSDNode::isValueValidForType(SVT, CFP->getValueAPF()) &&
+      if (ConstantFPSDNode::isValueValidForType(SVT, APF) &&
           // Only do this if the target has a native EXTLOAD instruction from
           // smaller type.
           TLI.isLoadExtLegal(ISD::EXTLOAD, OrigVT, SVT) &&
           TLI.ShouldShrinkFPConstant(OrigVT)) {
         Type *SType = SVT.getTypeForEVT(*DAG.getContext());
         LLVMC = cast<ConstantFP>(ConstantExpr::getFPTrunc(LLVMC, SType));
         VT = SVT;
         Extend = true;
       }
     }
+  }
 
   SDValue CPIdx =
       DAG.getConstantPool(LLVMC, TLI.getPointerTy(DAG.getDataLayout()));
   unsigned Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlignment();
   if (Extend) {
     SDValue Result = DAG.getExtLoad(
         ISD::EXTLOAD, dl, OrigVT, DAG.getEntryNode(), CPIdx,
         MachinePointerInfo::getConstantPool(DAG.getMachineFunction()), VT,

lib/Target/SystemZ/SystemZISelLowering.h

public:
   ISD::NodeType getExtendForAtomicOps() const override {
     return ISD::ANY_EXTEND;
   }
 
   bool supportSwiftError() const override {
     return true;
   }
 
+  bool ShouldShrinkSNaNConstant() const override {
+    return false;
+  }
+
 private:
   const SystemZSubtarget &Subtarget;
 
   // Implement LowerOperation for individual opcodes.
   SDValue lowerSETCC(SDValue Op, SelectionDAG &DAG) const;
   SDValue lowerBR_CC(SDValue Op, SelectionDAG &DAG) const;
   SDValue lowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;
   SDValue lowerGlobalAddress(GlobalAddressSDNode *Node,

test/CodeGen/SystemZ/fp-const-10.ll

+; Test loads of SNaN.
+;
+; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s
+
+; Test that we don't do an FP extending load, as this would result in a
+; converstion to QNaN.
+define double @f1() {
+; CHECK-LABEL: .LCPI0_0
+; CHECK:      .quad   9219994337134247936
+; CHECK-LABEL: f1:
+; CHECK:      larl    %r1, .LCPI0_0
+; CHECK-NOT:  ldeb    %f0, 0(%r1)
+; CHECK:      ld      %f0, 0(%r1)
+  ret double 0x7FF4000000000000
+}