This is an archive of the discontinued LLVM Phabricator instance.

Set FLT_EVAL_METHOD to -1 when fast-math is enabled.
ClosedPublic

Authored by zahiraam on Mar 7 2022, 7:46 AM.

Details

Summary

Currently the control of the eval-method is mixed with fast-math. FLT_EVAL_METHOD tells the user the precision at which, temporary results
are evaluated but when fast-math is enabled, the numeric values are not guaranteed to match the source semantics, so the eval-method is
meaningless.
For example, the expression x + y + z has as source semantics (x + y) + z. FLT_EVAL_METHOD is telling the user at which precision (x + y)
is evaluated. With fast-math enable the compiler may choose to evaluate the expression as (y + z) + x.

The correct behavior is to set the FLT_EVAL_METHOD to -1, to tell the user that the precision of the intermediate values is unknow.
This patch is doing that.

Diff Detail

Event Timeline

zahiraam created this revision.Mar 7 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 7:46 AM
zahiraam requested review of this revision.Mar 7 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 7:46 AM

The fix for the eval_method crash should be moved to a separate patch. Otherwise, this looks good. I have only minor comments.

clang/lib/Lex/PPMacroExpansion.cpp
1600

It looks like you've got tabs in the code here.

clang/lib/Sema/SemaAttr.cpp
521

Why don't you need the reverse of this in the Precise and Pop cases? Also, why is it necessary to call getCurrentFPEvalMethod() immediately after setting it?

clang/test/CodeGen/eval-method-fast-math.c
56

Can you add a test that uses the float_control push and pop?

zahiraam updated this revision to Diff 413936.Mar 8 2022, 2:24 PM
zahiraam edited the summary of this revision. (Show Details)
zahiraam marked 3 inline comments as done.
zahiraam added inline comments.
clang/lib/Lex/PPMacroExpansion.cpp
1600

No. I think that just means that the block of code was shifted by a few spaces because it's in a condition now.

clang/lib/Sema/SemaAttr.cpp
521

The call to getCurrentFPEvalMethod() was just left over debugging. Removed it.

zahiraam updated this revision to Diff 414216.Mar 9 2022, 2:11 PM
aaron.ballman added inline comments.Mar 10 2022, 10:59 AM
clang/lib/Sema/Sema.cpp
258

Shouldn't this be looking at getLangOpts().FastMath?

zahiraam added inline comments.Mar 10 2022, 11:12 AM
clang/lib/Sema/Sema.cpp
258

when the -ffast-math is enabled on the command line, it triggers all these math driver options:

"-menable-no-infs" "-menable-no-nans" "-fapprox-func" "-menable-unsafe-fp-math" "-fno-signed-zeros" "-mreassociate" "-freciprocal-math" "-ffp-contract=fast" "-ffast-math" "-ffinite-math-only"

That's a bit restrictive. We want the eval-method set to -1 when either reassoc or allowrecip are enabled.

aaron.ballman added inline comments.Mar 10 2022, 11:16 AM
clang/lib/Sema/Sema.cpp
258

Okay, then I think the comments about fast math should be fixed up; otherwise that's going to get confusing.

zahiraam updated this revision to Diff 414460.Mar 10 2022, 12:31 PM
zahiraam updated this revision to Diff 414461.
zahiraam marked an inline comment as done.
aaron.ballman added inline comments.Mar 10 2022, 12:41 PM
clang/test/CodeGen/eval-method-fast-math.c
84

What should happen in a case like this?

int whatever(float a, float b, float c) {
  #pragma float_control(precise, off)
  res = a + b + c;
  int val = __FLT_EVAL_METHOD__;
  #pragma float_control(precise, on)
  return __FLT_EVAL_METHOD__;
}

I would expect that int val would hold -1 and the return statement would return 0?

zahiraam added inline comments.Mar 10 2022, 1:10 PM
clang/test/CodeGen/eval-method-fast-math.c
84

This test case is failing with this error (at the second pragma):

t1.cpp:9:11: error: '#pragma float_control' can only appear at file scope or at
    the start of a compound statement
#pragma float_control(precise, off)
        ^
1 error generated.

Tried this:

int whatever(float a, float b, float c) {
#pragma float_control(precise, off)
res = a + b + c;
int val =__FLT_EVAL_METHOD__;
{
#pragma float_control(precise, on)
return __FLT_EVAL_METHOD__;
}

}

This generated these last IR instructions:

store float %add1, float* @"?res@@3MA", align 4
store i32 -1, i32* %val, align 4
ret i32 -1

Not sure if this is correct. I guess the first pragma (off) didn't popped, so it's still on? But inside the scope of the second pragma the first pragma shouldn't be visible? Not sure what should happen here.

aaron.ballman added inline comments.Mar 11 2022, 4:00 AM
clang/test/CodeGen/eval-method-fast-math.c
84

Not sure if this is correct.

It looks incorrect to me. What I'd expect is:

int whatever(float a, float b, float c) {
  #pragma float_control(precise, off)
  res = a + b + c;
  int val =__FLT_EVAL_METHOD__; // Expect -1
  {
    #pragma float_control(precise, on)
    return __FLT_EVAL_METHOD__; // Expect 0
  }
  return __FLT_EVAL_METHOD__; // Expect -1
}

I forgot about the function scope restrictions, but the file scope still has an interesting case worth testing:

float a = 1.0f, b = 2.0f, c = 3.0f;
#pragma float_control(precise, off)
float res = a + b + c;
int off =__FLT_EVAL_METHOD__; // Expect -1
#pragma float_control(precise, on)
float other_res = a + b + c;
int on = __FLT_EVAL_METHOD__; // Expect 0

Both of these are based on the idea that the float control pragma sets the flag until it's either popped (by leaving a compound statement) or it's changed to something else (by another pragma).

zahiraam updated this revision to Diff 414729.Mar 11 2022, 12:23 PM
zahiraam marked an inline comment as done.

It looks like there are some clang-format issues to handle as well.

clang/test/CodeGen/eval-method-fast-math.c
114–117

Shouldn't there be CHECK lines for these?

zahiraam added inline comments.Mar 11 2022, 1:34 PM
clang/test/CodeGen/eval-method-fast-math.c
114–117

These are the ones:
// CHECK: store i32 0

// CHECK: ret i32 -1

they are truncated. They are in reality these:

store i32 -1, i32* %val1, align 4
store i32 0, i32* %val2, align 4

Is that what you mean?

zahiraam updated this revision to Diff 414739.Mar 11 2022, 1:38 PM
aaron.ballman accepted this revision.Mar 14 2022, 5:11 AM

Please fix the clang format issue as well. Other than that and a nit, this LGTM.

clang/lib/Lex/PPMacroExpansion.cpp
1587

Pretty sure 2 can be 1 so that we only create an array of one token, right?

This revision is now accepted and ready to land.Mar 14 2022, 5:11 AM
zahiraam updated this revision to Diff 416168.Mar 17 2022, 7:02 AM
zahiraam marked an inline comment as done.
zahiraam updated this revision to Diff 416180.Mar 17 2022, 8:07 AM
bjope added a subscriber: bjope.Mar 23 2022, 6:18 AM

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

bjope added a subscriber: uabelho.Mar 23 2022, 6:19 AM

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Sorry for the troubles!

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

The way I read the requirements in C2x 7.12p3 are that the types float_t and double_t are always defined in <math.h>, but in the event FLT_EVAL_METHOD isn't 0, 1, or 2, the implementation has to pick whatever type makes the most sense for the underlying types.

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

The standard doesn't admit that fast math is a thing; it's up to the implementations to define what their fast math extension does.

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

I think it should still be handled in <math.h>. *I am not a C floating point expert, so take this suggestion with a grain of salt*: I think it is defensible to fallback to defining float_t as float and double_t as double when the eval method is indeterminable. @andrew.w.kaylor may have more nuanced thoughts here.

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Sorry for the troubles!

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

The way I read the requirements in C2x 7.12p3 are that the types float_t and double_t are always defined in <math.h>, but in the event FLT_EVAL_METHOD isn't 0, 1, or 2, the implementation has to pick whatever type makes the most sense for the underlying types.

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

The standard doesn't admit that fast math is a thing; it's up to the implementations to define what their fast math extension does.

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

I think it should still be handled in <math.h>. *I am not a C floating point expert, so take this suggestion with a grain of salt*: I think it is defensible to fallback to defining float_t as float and double_t as double when the eval method is indeterminable. @andrew.w.kaylor may have more nuanced thoughts here.

Thanks for the answer @aaron.ballman.
Looking at this https://godbolt.org/z/drhGEjvns i see that float_t is defined in math.h (if I remove the include it will fail). Not sure what version of libs godbolt is using, but I tend to agree that it should be defined in math.h too. But Andy will have a more definite answer.

bjope added a comment.Mar 23 2022, 8:06 AM

Looked around a bit.

So maybe I need to patch our newlib version somehow to make sure float_t is defined even for -1. But there could be problems with the LLVM libc implementation of math.h (which I think uses float_t.h).