This is an archive of the discontinued LLVM Phabricator instance.

Include math-errno with fast-math
ClosedPublic

Authored by zahiraam on May 31 2023, 1:12 PM.

Details

Diff Detail

Event Timeline

zahiraam created this revision.May 31 2023, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 1:12 PM
zahiraam requested review of this revision.May 31 2023, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 1:12 PM
zahiraam updated this revision to Diff 527950.Jun 2 2023, 1:36 PM
andrew.w.kaylor added inline comments.Jun 2 2023, 2:48 PM
clang/include/clang/Basic/FPOptions.def
30

Does this mean MathErrno is tracked in both LangOpts and FPOptions?

clang/lib/CodeGen/CGBuiltin.cpp
2285

You should add a comment here explaining what this is doing. I don't think it's obvious apart from the description of this patch.

clang/lib/CodeGen/CGCall.cpp
2429

I don't understand what this is doing.

clang/lib/CodeGen/CodeGenModule.h
609

Why is this a module level flag, and why does it default to true?

clang/test/CodeGen/math-errno.c
3

Isn't math-errno true by default when fast-math isn't used?

50

I think the 'afn' flag here is a problem. The backend has no concept of errno, so 'afn' will be treated as allowing the function to be replaced.

andrew.w.kaylor added inline comments.Jun 2 2023, 2:48 PM
clang/test/CodeGen/math-errno.c
34

Can you add a runline with -O0. That should prevent all instances of the intrinsics, right?

zahiraam updated this revision to Diff 530040.Jun 9 2023, 11:49 AM
zahiraam marked 5 inline comments as done.
zahiraam added inline comments.
clang/include/clang/Basic/FPOptions.def
30

Yes. So that we can check that if it's overridden by a pragma or an attribute. I don't see another way.

clang/test/CodeGen/math-errno.c
34

Yes.

50

Added all the commands that are triggered by fast-math in the RUN line command.

clang/lib/CodeGen/CGBuiltin.cpp
2286

Since we handle "#pragma float_control" for all targets, we should be consistent. I don't think the reference to MSVC here is useful.

I'm not sure why MSVC thinks it needs to call the sqrtf() function at O1, but I don't necessarily think we should follow that lead. If we generate the intrinsic here, the optimizer will make its own decisions about O1 versus O2, etc. There's no reason that errno should be preserved at O1 if it's been disabled with -fno-math-errno or we're enabled fast-math.

2300

I'm confused by this condition. If MathErrnoOverride is true, don't we need to set errno regardless of the optimization level or fast-math setting?

2303

Why do we need to check FastMath if the function is marked as optnone?

2324

Based on the comment above, if the function call is marked as const, then it will never set errno, so I don't think we need to check EmitIntrinsic in that case.

clang/test/CodeGen/math-errno.c
29

Shouldn't the precise pragma remove the nofpcalss(nan inf) attributes? The definition of setFPPreciseEnabled looks like it's trying to do that. Maybe we need another patch to handle that?

41

Again, this may be beyond the scope of your change, but it seems like '#pragma float_control(precise, off) should lead to fast-math flags being set on this call.

44

I expected the intrinsic to be called in this case. Do you know why it isn't? I believe it is without your change.

47

I didn't expect to see the intrinsic in this case. If we use the intrinsic, the x86-backend will generate sqrtss instead of the function call, and I think that is wrong.

57

Again, I didn't expect the intrinsic with optnone in any of these cases.

zahiraam updated this revision to Diff 539961.Jul 13 2023, 4:51 AM
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited reviewers, added: rjmccall, sepavloff; removed: jcranmer-intel.
zahiraam marked 6 inline comments as done and an inline comment as not done.
zahiraam updated this revision to Diff 542141.Jul 19 2023, 12:03 PM
zahiraam added inline comments.
clang/test/CodeGen/math-errno.c
3

Yes, that's correct. This RUN line simulates compilation at -O2. The following cc1 options are triggered: -fmath-errno -ffp-contract=on
-fno-rounding-math -O2. That's what I am using here.

29

Shouldn't the precise pragma remove the nofpcalss(nan inf) attributes? The definition of setFPPreciseEnabled looks like it's trying to do that. Maybe we need another patch to handle that?

I will make a note of it and work on it in a subsequent patch.

41

Again, this may be beyond the scope of your change, but it seems like '#pragma float_control(precise, off) should lead to fast-math flags being set on this call.

Yes. Making a note of it.

Review anyone please? Thanks.

aaron.ballman added inline comments.
clang/include/clang/Basic/FPOptions.def
29–30

Shouldn't this one be Float16ExcessPrecision? (Are we missing test coverage that would have caught that?)

clang/include/clang/Basic/LangOptions.h
857

Everything else does !Value; is it intentional that you're using Value instead?

clang/lib/CodeGen/CGBuiltin.cpp
2321–2328

I think this has gotten sufficiently complex that it might be worth splitting the logic out a bit more:

bool Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone;
if (!Optimize) Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone;
... and so on ...

WDYT?

zahiraam added inline comments.Jul 25 2023, 12:23 PM
clang/include/clang/Basic/FPOptions.def
29–30

oops! the test would be for the BFloat16ExcessPrecision, right?

clang/include/clang/Basic/LangOptions.h
857

Yes. I want to set the value of math-errno to the value of the pragma in the source.

aaron.ballman added inline comments.Jul 25 2023, 12:31 PM
clang/include/clang/Basic/FPOptions.def
29–30

Yup -- that can be handled in a separate patch as it doesn't really have much to do with this one -- just something I noticed as a drive-by.

clang/include/clang/Basic/LangOptions.h
857

Hmm, okay. Making sure I understand the logic, please excuse me if this is a dumb question. :-) So the idea here is that if fp_precise is on, we have to honor math errno, and if fp_precise is off, we can ignore math errno?

clang/lib/CodeGen/CGBuiltin.cpp
2297

Is the part about -O2 accurate? We just check not -O0 in the code.

zahiraam marked 2 inline comments as done.Jul 25 2023, 12:39 PM
zahiraam added inline comments.
clang/include/clang/Basic/LangOptions.h
857

Correct.

zahiraam added inline comments.Jul 25 2023, 12:48 PM
clang/lib/CodeGen/CGBuiltin.cpp
2321–2328

I understand I need to split the logic, but I don't get your proposal here. Do you mean?

bool Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone;
if (!Optimize) Optimize = !getLangOpts().MathErrno && !ErrnoOverriden && !OptNone;
... and so on ...
zahiraam updated this revision to Diff 544084.Jul 25 2023, 1:14 PM
aaron.ballman added inline comments.Jul 27 2023, 5:38 AM
clang/lib/CodeGen/CGBuiltin.cpp
2321–2328

Oops, yeah, that a was copy pasta mistake on my part. You've got the right idea -- mostly just split up the logic and add comments explaining why the predicates exist.

zahiraam updated this revision to Diff 544745.Jul 27 2023, 6:41 AM
zahiraam marked 3 inline comments as done.
zahiraam added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
2321–2328

Changed the name of the variable you proposed. Is that OK?

aaron.ballman added inline comments.Jul 27 2023, 9:19 AM
clang/lib/CodeGen/CGBuiltin.cpp
2321–2328

Totally fine by me, but there's some more logic that you can strip out of the if statement, and it'd help me out if there were some comments explaining why each of these blocks might disable generating intrinsics.

zahiraam updated this revision to Diff 544893.Jul 27 2023, 12:40 PM
aaron.ballman added inline comments.Jul 28 2023, 5:37 AM
clang/lib/CodeGen/CGBuiltin.cpp
2318–2320
zahiraam updated this revision to Diff 545666.Jul 31 2023, 7:54 AM
zahiraam marked an inline comment as done.
aaron.ballman added inline comments.Aug 1 2023, 6:44 AM
clang/docs/LanguageExtensions.rst
4698 ↗(On Diff #545666)
4702 ↗(On Diff #545666)
clang/docs/UsersManual.rst
1727 ↗(On Diff #545666)

Can you re-flow this to 80 columns?

zahiraam updated this revision to Diff 546055.Aug 1 2023, 7:28 AM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
clang/docs/LanguageExtensions.rst
4702 ↗(On Diff #545666)

At least I was consistent :-)

clang/docs/UsersManual.rst
1727 ↗(On Diff #545666)

Did the same for the rest of the text too. Is that OK?

aaron.ballman added inline comments.Aug 1 2023, 7:45 AM
clang/docs/UsersManual.rst
1727 ↗(On Diff #545666)

Yeah, that's not the end of the world (if it was touching the whole file, that should be done as an NFC change outside of this patch, but these changes are related enough I think it's fine to do in this patch).

aaron.ballman accepted this revision.Sep 6 2023, 6:12 AM

LGTM, but please add a release note describing the changes when you land them.

This revision is now accepted and ready to land.Sep 6 2023, 6:12 AM
zahiraam updated this revision to Diff 556029.Sep 6 2023, 7:06 AM

LGTM, but please add a release note describing the changes when you land them.

@aaron.ballman thanks for the reviews. Added some note in the RN, let me know if that is enough.

LGTM, but please add a release note describing the changes when you land them.

@aaron.ballman thanks for the reviews. Added some note in the RN, let me know if that is enough.

Release note looks great, thank you!

zahiraam updated this revision to Diff 556057.Sep 6 2023, 11:07 AM

@andrew.w.kaylor Would you mind taking a last look at this before I can push this, please? I got the thumbs up from Aaron.

zahiraam updated this revision to Diff 556141.Sep 7 2023, 6:43 AM
This revision was landed with ongoing or failed builds.Sep 8 2023, 6:49 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 8 2023, 11:17 AM

This breaks check-clang on macOS: http://45.33.8.238/macm1/68762/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This breaks check-clang on macOS: http://45.33.8.238/macm1/68762/step_7.txt

Please take a look and revert for now if it takes a while to fix.

I have a fix for it here: https://reviews.llvm.org/D159486

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.

I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.

I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

This looks like it's happening only for -Oi (i>=1) and on Linux only?

[...]

I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

This looks like it's happening only for -Oi (i>=1) and on Linux only?

Yes I haven't seen it with -O0. I also haven't seen it for my out-of-tree target, only for linux/X86.

What's the way forward here? Revert if it's unclear what to do?

What's the way forward here? Revert if it's unclear what to do?

We have a fix for it. By eod a PR will be created. Thanks.

What's the way forward here? Revert if it's unclear what to do?

We have a fix for it. By eod a PR will be created. Thanks.

Ok, nice. Thanks!