[clang] Use constant rounding mode for floating literals #90877

spavloff · 2024-05-02T17:12:56Z

Conversion of floating-point literal to binary representation must be made using constant rounding mode, which can be changed using pragma FENV_ROUND. For example, the literal "0.1F" should be representes by either 0.099999994 or 0.100000001 depending on the rounding direction.

llvmbot · 2024-05-02T17:13:29Z

@llvm/pr-subscribers-clang

Author: Serge Pavlov (spavloff)

Changes

Conversion of floating-point literal to binary representation must be made using constant rounding mode, which can be changed using pragma FENV_ROUND. For example, the literal "0.1F" should be representes by either 0.099999994 or 0.100000001 depending on the rounding direction.

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

4 Files Affected:

(modified) clang/include/clang/Lex/LiteralSupport.h (+4-6)
(modified) clang/lib/Lex/LiteralSupport.cpp (+3-3)
(modified) clang/lib/Sema/SemaExpr.cpp (+2-1)
(modified) clang/test/AST/const-fpfeatures.c (+6)

diff --git a/clang/include/clang/Lex/LiteralSupport.h b/clang/include/clang/Lex/LiteralSupport.h
index 2ed42d1c5f9aae..705021fcfa5b11 100644
--- a/clang/include/clang/Lex/LiteralSupport.h
+++ b/clang/include/clang/Lex/LiteralSupport.h
@@ -118,12 +118,10 @@ class NumericLiteralParser {
   /// bits of the result and return true.  Otherwise, return false.
   bool GetIntegerValue(llvm::APInt &Val);
 
-  /// GetFloatValue - Convert this numeric literal to a floating value, using
-  /// the specified APFloat fltSemantics (specifying float, double, etc).
-  /// The optional bool isExact (passed-by-reference) has its value
-  /// set to true if the returned APFloat can represent the number in the
-  /// literal exactly, and false otherwise.
-  llvm::APFloat::opStatus GetFloatValue(llvm::APFloat &Result);
+  /// Convert this numeric literal to a floating value, using the specified
+  /// APFloat fltSemantics (specifying float, double, etc) and rounding mode.
+  llvm::APFloat::opStatus GetFloatValue(llvm::APFloat &Result,
+                                        llvm::RoundingMode RM);
 
   /// GetFixedPointValue - Convert this numeric literal value into a
   /// scaled integer that represents this value. Returns true if an overflow
diff --git a/clang/lib/Lex/LiteralSupport.cpp b/clang/lib/Lex/LiteralSupport.cpp
index 9c0cbea5052cb2..3df0391bdda772 100644
--- a/clang/lib/Lex/LiteralSupport.cpp
+++ b/clang/lib/Lex/LiteralSupport.cpp
@@ -1520,7 +1520,8 @@ bool NumericLiteralParser::GetIntegerValue(llvm::APInt &Val) {
 }
 
 llvm::APFloat::opStatus
-NumericLiteralParser::GetFloatValue(llvm::APFloat &Result) {
+NumericLiteralParser::GetFloatValue(llvm::APFloat &Result,
+                                    llvm::RoundingMode RM) {
   using llvm::APFloat;
 
   unsigned n = std::min(SuffixBegin - ThisTokBegin, ThisTokEnd - ThisTokBegin);
@@ -1534,8 +1535,7 @@ NumericLiteralParser::GetFloatValue(llvm::APFloat &Result) {
     Str = Buffer;
   }
 
-  auto StatusOrErr =
-      Result.convertFromString(Str, APFloat::rmNearestTiesToEven);
+  auto StatusOrErr = Result.convertFromString(Str, RM);
   assert(StatusOrErr && "Invalid floating point representation");
   return !errorToBool(StatusOrErr.takeError()) ? *StatusOrErr
                                                : APFloat::opInvalidOp;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 0cca02c338954a..db96c8692f2516 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -3858,7 +3858,8 @@ static Expr *BuildFloatingLiteral(Sema &S, NumericLiteralParser &Literal,
   using llvm::APFloat;
   APFloat Val(Format);
 
-  APFloat::opStatus result = Literal.GetFloatValue(Val);
+  APFloat::opStatus result =
+      Literal.GetFloatValue(Val, S.CurFPFeatures.getRoundingMode());
 
   // Overflow is always an error, but underflow is only an error if
   // we underflowed to zero (APFloat reports denormals as underflow).
diff --git a/clang/test/AST/const-fpfeatures.c b/clang/test/AST/const-fpfeatures.c
index 6600ea27405d9c..247245c776fed1 100644
--- a/clang/test/AST/const-fpfeatures.c
+++ b/clang/test/AST/const-fpfeatures.c
@@ -19,6 +19,9 @@ float FI1u = 0xFFFFFFFFU;
 float _Complex C1u = C0;
 // CHECK: @C1u = {{.*}} { float, float } { float 0x3FF0000020000000, float 0x3FF0000020000000 }
 
+float FLu = 0.1F;
+// CHECK: @FLu = {{.*}} float 0x3FB9999980000000
+
 
 #pragma STDC FENV_ROUND FE_DOWNWARD
 
@@ -35,3 +38,6 @@ float FI1d = 0xFFFFFFFFU;
 
 float _Complex C1d = C0;
 // CHECK: @C1d = {{.*}} { float, float } { float 1.000000e+00, float 1.000000e+00 }
+
+float FLd = 0.1F;
+// CHECK: @FLd = {{.*}} float 0x3FB99999A0000000

erichkeane

probably needs a release note

erichkeane · 2024-05-02T21:08:52Z

clang/lib/Sema/SemaExpr.cpp

@@ -3858,7 +3858,8 @@ static Expr *BuildFloatingLiteral(Sema &S, NumericLiteralParser &Literal,
  using llvm::APFloat;
  APFloat Val(Format);

-  APFloat::opStatus result = Literal.GetFloatValue(Val);
+  APFloat::opStatus result =


Does this work right in templates? Can we get a test that validates that a NTTP or Dependent value where the rounding mode is set either around or inside the function does the math right?

erichkeane · 2024-05-02T21:10:07Z

Also, test failures are related to this patch!

erichkeane · 2024-05-03T15:00:05Z

clang/test/AST/const-fpfeatures.cpp

+  float value;
+};
+
+#pragma STDC FENV_ROUND FE_DOWNWARD


I meant something like:

template<typename T, T V> void foo() { #pragma STDC FENV_ROUND FE_DOWNWARD T Val = V; // Should be: { float 0x3FB9999980000000 } } #pragma STDC FENV_ROUND FE_UPDWARD foo<float, 0.1F>();

Ah, I see that github ate the first line of my comment, see above, i'v eedited it.

In this snippet Val in foo<float, 0.1F>() should be float 0x3FB99999A0000000 (rounded upward), because the literal 0.1F is in the region, where #pragma STDC FENV_ROUND FE_UPDWARD is in effect. The body of foo does not contain literals.

In this snippet Val in foo<float, 0.1F>() should be float 0x3FB99999A0000000 (rounded upward), because the literal 0.1F is in the region, where #pragma STDC FENV_ROUND FE_UPDWARD is in effect. The body of foo does not contain literals.

Hmm... ok, then the feature isn't quite what I expected it was. Does the FENV_ROUND exclusively change the behavior of literals? I thought it messed with math as well? (I thought it was on assignment, but perhaps not!).

I'm more concerned about a case where the point-of-instantiation and the template have different rounding modes. But I see this patch is exclusively for literals, so we can let that go.

One thing to ensure: can we have an AST test (like your 'foo' example) that shows that they are different instantiations? Are there any cases where we could get two instantiation-requests spelled differently, but because of different rounding modes, are the same instantiation?

Does the FENV_ROUND exclusively change the behavior of literals? I thought it messed with math as well?

Of course, FENV_ROUND affects any math, but this patch fixes misbehavior of literals only. And the note was about different expectation.

Assignment isn't an operation that depends on rounding mode, but changing the snippet to such:

template<typename T, T V> void foo() { #pragma STDC FENV_ROUND FE_DOWNWARD T Val = V + 1; }

would make use of the rounding mode. In this case the addition in foo would be evaluated using rounding down, but the literal is converted to AST object using rounding up.

I'm more concerned about a case where the point-of-instantiation and the template have different rounding modes.

Does this test address your concern: https://github.com/spavloff/llvm-project/blob/aeb6074444513587924106081213335f73ba6eb0/clang/test/AST/ast-dump-fpfeatures.cpp#L124-L143 ?

One thing to ensure: can we have an AST test (like your 'foo' example) that shows that they are different instantiations? Are there any cases where we could get two instantiation-requests spelled differently, but because of different rounding modes, are the same instantiation?

All instantiations of a function template use the rounding mode specified in the template definition. Otherwise we would have bad things like two instantiations in different translation units that use different rounding modes. The rounding mode is attached to AST nodes in the AST representation for function templates, tests in AST/ast-dump-fpfeatures.cpp check this property.

Does this test address your concern: https://github.com/spavloff/llvm-project/blob/aeb6074444513587924106081213335f73ba6eb0/clang/test/AST/ast-dump-fpfeatures.cpp#L124-L143 ?

That one does not. I'm more looking for:

template<typename T, T f> void foo(){} #pragma STDC FENV_ROUND FE_DOWNWARD foo<float, 0.1f>(); #pragma STDC FENV_ROUND FE_UPWARD foo<float, 0.1f>();

Then confirming that there are two separate instantiations of foo.

All instantiations of a function template use the rounding mode specified in the template definition. Otherwise we would have bad things like two instantiations in different translation units that use different rounding modes.

Right, that was very much my concern, the ODR implications of that are scary. So jsut the 1 test above, and I think I am good about this, though having @jcranmer-intel and/or @zahiraam review would also be appreciated.

The interaction of these pragmas with C++ is underspecified, although for the most part, you can make pretty reasonable expectations about what the results should be. I'm also planning on bringing a paper to C++ to clarify the semantics somewhat, with the intent mostly being "just don't allow the pragma in most places."

That said, templates do add in all sorts of fun extra complexity, and there are at least three scenarios worth testing (with regards to interpretation of constants within the template body):

Implicit template instantiation (you already test this)

Explicit template instantiation

Template specialization

I can definitely agree that implicit template instantiation should inherit from the rounding mode of the definition. Template specialization probably shouldn't inherit from the original template definition. Explicit instantiation... I'm not sure? I can argue that one either way.

Explicit instantiation also should use the rounding mode at the point of definition. It is possible that the template is instantiated in two translation units and rounding mode is different at the points of instantiation.

jcranmer-intel · 2024-05-03T19:18:48Z

I've been doing some testing, and I do want to confirm my understanding of the C standard here.

From what I can tell, macro expansion (phase 4) happens before constants are parsed (phase 7). As a result, if you have code like this:

#define CONSTANT 0.1f

the interpretation of 0.1f depends on the state of the pragma at point of use of the macro, not point of declaration of the macro. Given that pragmas can be defined with _Pragma, with lambdas or statement expressions (albeit neither of which is standard C), it should be possible to create a macro that evaluates a constant with a given rounding mode:

#define rendevous(x) _Pragma(#x)
#define CONSTANT(RM, x) ([](){ rendevous(STDC FENV_ROUND RM); return x; })())

This does seem to indeed be the behavior of the current implementation, but I would like to see some tests in the test code confirming that it's the macro use, not the #define that determines the interpretation of the constant, and another test for the _Pragma case.

github-actions · 2024-05-04T13:51:16Z

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

github-actions · 2024-05-04T13:52:08Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 1aeb64c8ec7b96b2301929d8a325a6e1d9ddaa2f cf6ff55ad4a45875a821b3ac82c22bb7917b4d67 -- clang/include/clang/Lex/LiteralSupport.h clang/lib/Lex/LiteralSupport.cpp clang/lib/Sema/SemaExpr.cpp clang/test/AST/const-fpfeatures.c clang/test/AST/const-fpfeatures.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index da107a6844..a344d1281e 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -3861,8 +3861,7 @@ static Expr *BuildFloatingLiteral(Sema &S, NumericLiteralParser &Literal,
   llvm::RoundingMode RM = S.CurFPFeatures.getRoundingMode();
   if (RM == llvm::RoundingMode::Dynamic)
     RM = llvm::RoundingMode::NearestTiesToEven;
-  APFloat::opStatus result =
-      Literal.GetFloatValue(Val, RM);
+  APFloat::opStatus result = Literal.GetFloatValue(Val, RM);
 
   // Overflow is always an error, but underflow is only an error if
   // we underflowed to zero (APFloat reports denormals as underflow).

jcranmer-intel

I'm generally happy with the testing and semantics at this point.

spavloff · 2024-05-16T04:25:13Z

Are there any other comments? Can this PR be considered as approved?

AaronBallman

The changes should come with a release note so that users know about the behavioral change, but otherwise LGTM.

spavloff · 2024-05-16T14:14:46Z

The changes should come with a release note so that users know about the behavioral change, but otherwise LGTM.

Pragma FENV_ROUND is still officially unsupported, compiler issues a warning about this. It is #89617 that makes FENV_ROUND supported, it requires release note.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 2, 2024

shafik requested review from AaronBallman, erichkeane and jcranmer-intel May 2, 2024 21:05

erichkeane reviewed May 2, 2024

View reviewed changes

erichkeane requested a review from zahiraam May 2, 2024 21:09

spavloff added 3 commits May 3, 2024 13:45

Fix typo

a6fad25

Add test for NTTP

f2e0414

Replace dynamic rounding mode with NearestTiesToEven

cf6ff55

erichkeane reviewed May 3, 2024

View reviewed changes

spavloff added 2 commits May 3, 2024 22:16

Add additional test for float literal as NTTP

aeb6074

Fix clang-format error

ed25d8a

spavloff requested a review from rjmccall May 3, 2024 17:06

Add test for template instatiations

da2795c

spavloff added 3 commits May 5, 2024 00:43

Add test for literal in macro

58cbc15

Reorder tests

50173a8

Add tests for explicit instantiation and specialization

6ce8363

jcranmer-intel reviewed May 10, 2024

View reviewed changes

spavloff mentioned this pull request May 13, 2024

Implementation of '#pragma STDC FENV_ROUND' #89617

Open

AaronBallman approved these changes May 16, 2024

View reviewed changes

spavloff merged commit f4066fa into llvm:main May 17, 2024

spavloff deleted the round.literal branch May 17, 2024 05:08

[clang] Use constant rounding mode for floating literals #90877

[clang] Use constant rounding mode for floating literals #90877

Uh oh!

Conversation

spavloff commented May 2, 2024

Uh oh!

llvmbot commented May 2, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane commented May 2, 2024

Uh oh!

erichkeane May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

jcranmer-intel commented May 3, 2024

Uh oh!

github-actions bot commented May 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 4, 2024

Uh oh!

jcranmer-intel left a comment

Choose a reason for hiding this comment

Uh oh!

spavloff commented May 16, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

spavloff commented May 16, 2024

Uh oh!

Uh oh!

erichkeane May 3, 2024 •

edited

Loading

github-actions bot commented May 4, 2024 •

edited

Loading