Page MenuHomePhabricator

[APFloat] convert SNaN to QNaN in convert() and raise Invalid signal
ClosedPublic

Authored by spatel on Sep 24 2020, 9:16 AM.

Details

Summary

This is an alternate fix (see D87835) for a bug where a NaN constant gets wrongly transformed into Infinity via truncation.
In this patch, we uniformly convert any SNaN to QNaN while raising 'invalid op'.
But we don't have (AFAIK), a way to specify a 32-bit SNaN value in LLVM IR, so those are always encoded/decoded by calling convert from/to 64-bit hex.

Diff Detail

Event Timeline

spatel created this revision.Sep 24 2020, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 9:16 AM
spatel requested review of this revision.Sep 24 2020, 9:16 AM
spatel planned changes to this revision.Sep 24 2020, 9:24 AM

The clang test diffs got dropped from my diff...will update.

spatel updated this revision to Diff 294092.Sep 24 2020, 9:36 AM

Updated diff to include clang tests that fail with the setting of the quiet bit.

spatel updated this revision to Diff 294416.Sep 25 2020, 1:39 PM
spatel edited the summary of this revision. (Show Details)

Patch updated:
Here's an update to the better SNAN fix on top of the previous APFloat patch (we have confirmation that we are no longer miscompiling to Inf in PR43907).
I'm not sure how we would emulate the payload processing in APFloat::convert(), so I just fixed up the quiet bit after we call that.
I have not looked at the background on the MIPS tests, so I don't know if that's ok. The other test diffs appear as expected now.

Ooh, the MIPS case is weird...

Apparently someone, a while back, made a halfhearted effort at supporting the old MIPS NaN encoding, which reverses the distinction between SNaNs and QNaNs. See http://reviews.llvm.org/D7882 . Of course, it doesn't really work because constant folding doesn't understand the reversal.

Probably you should change clang/test/CodeGen/mips-unsupported-nan.c so it doesn't truncate the result of __builtin_nan(). Otherwise, you're not really making any more broken than it is already.

Ooh, the MIPS case is weird...

Apparently someone, a while back, made a halfhearted effort at supporting the old MIPS NaN encoding, which reverses the distinction between SNaNs and QNaNs. See http://reviews.llvm.org/D7882 . Of course, it doesn't really work because constant folding doesn't understand the reversal.

Probably you should change clang/test/CodeGen/mips-unsupported-nan.c so it doesn't truncate the result of __builtin_nan(). Otherwise, you're not really making any more broken than it is already.

Thanks for digging that up. I'll add code/comments to explain.

spatel updated this revision to Diff 294497.Sep 26 2020, 6:40 AM

Added code comments to explain the MIPS tests.

This revision is now accepted and ready to land.Sep 28 2020, 10:34 AM
spatel planned changes to this revision.Sep 29 2020, 7:01 AM
spatel added a subscriber: sepavloff.

D87822 landed recently ( cc @sepavloff ), so this patch is now failing on the clang tests with:

/Users/spatel/GitHub/llvm-project/clang/test/CodeGen/builtin-nan-legacy.c:6:3: error: initializer element is not a compile-time constant
  __builtin_nan(""),
  ^~~~~~~~~~~~~~~~~

D87822 landed recently ( cc @sepavloff ), so this patch is now failing on the clang tests with:

/Users/spatel/GitHub/llvm-project/clang/test/CodeGen/builtin-nan-legacy.c:6:3: error: initializer element is not a compile-time constant
  __builtin_nan(""),
  ^~~~~~~~~~~~~~~~~

I'm not sure what the correct fix will be. The cast from double to float in those tests is raising the exception (opInvalidOp) as expected. That then causes the new APFloat::opStatus check in checkFloatingPointResult() to trigger and cause the error/stop compiling.

D87822 landed recently ( cc @sepavloff ), so this patch is now failing on the clang tests with:

/Users/spatel/GitHub/llvm-project/clang/test/CodeGen/builtin-nan-legacy.c:6:3: error: initializer element is not a compile-time constant
  __builtin_nan(""),
  ^~~~~~~~~~~~~~~~~

I'm not sure what the correct fix will be. The cast from double to float in those tests is raising the exception (opInvalidOp) as expected. That then causes the new APFloat::opStatus check in checkFloatingPointResult() to trigger and cause the error/stop compiling.

I would propose to put a check into checkFloatingPointResult like:

if (St & APFloat::opStatus::opInvalidOp) {
  if (E is __builtin_nan() and probably other similar builtins)
    return true;
...

It would allow initialization with NaN but catches other operations with it.

I would propose to put a check into checkFloatingPointResult like:

if (St & APFloat::opStatus::opInvalidOp) {
  if (E is __builtin_nan() and probably other similar builtins)
    return true;
...

It would allow initialization with NaN but catches other operations with it.

That seems like a fragile fix - future changes to APFloat behavior might trigger the same error. Is there some reason we are not predicating the error based on the FP exception mode?

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 3bc649b9699..323de80a17d 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2439,7 +2439,8 @@ static bool checkFloatingPointResult(EvalInfo &Info, const Expr *E,
     return false;
   }
 
-  if (St & APFloat::opStatus::opInvalidOp) {
+  if (St & APFloat::opStatus::opInvalidOp &&
+      FPO.getFPExceptionMode() != LangOptions::FPE_Ignore) {
     // There is no usefully definable result.
     Info.FFDiag(E);
     return false;

I would propose to put a check into checkFloatingPointResult like:

if (St & APFloat::opStatus::opInvalidOp) {
  if (E is __builtin_nan() and probably other similar builtins)
    return true;
...

It would allow initialization with NaN but catches other operations with it.

That seems like a fragile fix - future changes to APFloat behavior might trigger the same error. Is there some reason we are not predicating the error based on the FP exception mode?

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 3bc649b9699..323de80a17d 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2439,7 +2439,8 @@ static bool checkFloatingPointResult(EvalInfo &Info, const Expr *E,
     return false;
   }
 
-  if (St & APFloat::opStatus::opInvalidOp) {
+  if (St & APFloat::opStatus::opInvalidOp &&
+      FPO.getFPExceptionMode() != LangOptions::FPE_Ignore) {
     // There is no usefully definable result.
     Info.FFDiag(E);
     return false;

I have no objection to such solution, if it enables this patch.

Catching Invalid exception in all situations but initialization looks more natural, but this is a topic for other patches.

spatel updated this revision to Diff 295009.Sep 29 2020, 8:49 AM

Patch updated:
Added a check for the FP exception mode before erroring out on APFloat::opStatus::opInvalidOp.

This revision is now accepted and ready to land.Sep 29 2020, 8:49 AM

The change to ExprConstant should be a different patch, with a test that we can still constant-evaluate something else that results in opInvalid (I assume there must be something?).

The change to ExprConstant should be a different patch, with a test that we can still constant-evaluate something else that results in opInvalid (I assume there must be something?).

I can't find an existing path to expose that difference in a test. We use checkFloatingPointResult() in 2 places: FP binops and FP cast. Without this patch, casting never sets opInvalidOp. Binops have a variety of means to create a NaN + opInvalidOp, but that path is gated by:

// [expr.pre]p4:
//   If during the evaluation of an expression, the result is not
//   mathematically defined [...], the behavior is undefined.
// FIXME: C++ rules require us to not conform to IEEE 754 here.
if (LHS.isNaN()) {
  Info.CCEDiag(E, diag::note_constexpr_float_arithmetic) << LHS.isNaN();
  return Info.noteUndefinedBehavior();
}
return checkFloatingPointResult(Info, E, St);

So unless there's a way to raise opInvalidOp without also creating a NaN, we're stuck?

So unless there's a way to raise opInvalidOp without also creating a NaN, we're stuck?

Maybe you can divide by zero? Otherwise, I agree, we can't split this into a separate patch.

But in any case, we should have some test coverage outside the MIPS-specific testcases.

So unless there's a way to raise opInvalidOp without also creating a NaN, we're stuck?

Maybe you can divide by zero? Otherwise, I agree, we can't split this into a separate patch.

div-by-zero has its own status bit set in IEEEFloat::divideSpecials():

case PackCategoriesIntoKey(fcNormal, fcZero):
  category = fcInfinity;
  return opDivByZero;

But in any case, we should have some test coverage outside the MIPS-specific testcases.

Ok - I think the tests will necessarily be similar (FP cast) to what we have already, but we can remove the added complexity of the MIPS option.

I think we can split it into a separate patch even if we have no way to test it yet. It's a significantly broader change, at least in theory.

spatel updated this revision to Diff 295345.Sep 30 2020, 10:32 AM

Patch updated:
I added the baseline tests with the NAN convert on non-MIPS targets here:
rG187686bea387
And I added a test comment to describe the difference from this patch along with the updated CHECK line.
The exception mode change is still here, so we can see the complete picture, but I will pre-commit that (still no test diff that I can find) if that's preferred.

Hmm. Is there a way to actually get an sNaN in floating-point type? I'm concerned that we might be pushing a formal argument in a way that undermines usability.

Hmm. Is there a way to actually get an sNaN in floating-point type? I'm concerned that we might be pushing a formal argument in a way that undermines usability.

I mean in a non-double floating-point type. I guess there's __builtin_nansf and so on; for some reason we don't seem to test those as much as we test __builtin_nans.

spatel updated this revision to Diff 295375.Sep 30 2020, 12:34 PM

Added tests that use __builtin_nanf and __builtin_nansf (no changes with this patch as a whole or split into 2 pieces). We allow creating 32-bit NAN directly with those builtins.

This may be confusing because LLVM IR doesn't print 32-bit FP as 32-bit hex. That's the FIXME note in AsmWriter.cpp and hopefully the next step...although I suspect it would mean updating several hundred regression tests.

efriedma accepted this revision.Sep 30 2020, 1:33 PM

LGTM, but please wait to see if @rjmccall has any other comments.

Please make sure the commit message has explanation has some explanation of what the clang change is doing.

spatel updated this revision to Diff 295584.Thu, Oct 1, 8:41 AM

Patch updated:
Split the clang AST diff off into its own patch and made this patch depend on it.

Herald added a project: Restricted Project. · View Herald TranscriptThu, Oct 1, 11:38 AM