Page MenuHomePhabricator

[FPEnv Core 02/14] Add FPEnv access flags to fast-math flags
AcceptedPublic

Authored by sdmitrouk on Oct 26 2015, 6:55 AM.

Details

Summary

Will be used to disable optimizations related to floating-point
operations which would affect runtime semantics (e.g. raising
floating-point exceptions).

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitrouk updated this revision to Diff 38400.Oct 26 2015, 6:55 AM
sdmitrouk retitled this revision from to [FPEnv Core 02/14] Add FPEnv access flags to fast-math flags.
sdmitrouk updated this object.
sdmitrouk added reviewers: hfinkel, mehdi_amini.
sdmitrouk set the repository for this revision to rL LLVM.
sdmitrouk added subscribers: llvm-commits, scanon, resistor.
scanon added inline comments.Oct 26 2015, 8:12 AM
docs/LangRef.rst
1998

Let's avoid "thrown", which tends to conjure an entirely different notion of "exception". The IEEE-754 nomenclature is "raised".

2005

It should explicitly imply that kexc and kround are *not set*.

I'd like to propose a mildly different design; I'd phrase the two flags using a positive perspective:

  • nexc: Assume that floating-point exceptions are not relevant.
  • nrnd: Assume that the rounding-mode is round-to-nearest (ties even).

This has the nice property that fast is the union of all flags and, in general, keeps things consistent with all the other fast math flags. The general pattern is that adding flags permits further optimization; places where this is violated have been a wellspring of bugs (namely volatile).

Older bitcode would have to be auto-upgraded to add those flags onto older instructions.

I'd like to propose a mildly different design; I'd phrase the two flags using a positive perspective:

  • nexc: Assume that floating-point exceptions are not relevant.
  • nrnd: Assume that the rounding-mode is round-to-nearest (ties even).

    This has the nice property that fast is the union of all flags and, in general, keeps things consistent with all the other fast math flags. The general pattern is that adding flags permits further optimization; places where this is violated have been a wellspring of bugs (namely volatile).

I like this ("observed" instead of "relevant", perhaps).

I'd like to propose a mildly different design; I'd phrase the two flags using a positive perspective:

  • nexc: Assume that floating-point exceptions are not relevant.
  • nrnd: Assume that the rounding-mode is round-to-nearest (ties even).

    This has the nice property that fast is the union of all flags and, in general, keeps things consistent with all the other fast math flags. The general pattern is that adding flags permits further optimization; places where this is violated have been a wellspring of bugs (namely volatile).

    Older bitcode would have to be auto-upgraded to add those flags onto older instructions.

There is a compatibility issue. kexc and kround can be added when they are needed leaving regular behaviour for cases where these flags are not specified, they also have property of having all fast-math flags unset by default. Combination of nexc and nrnd flags corresponds to current behaviour and all new code that wants folding will need to include these flags explicitly. Or is this what you mean by saying Older bitcode would have to be auto-upgraded to add those flags onto older instructions.?

Older bitcode would have to be auto-upgraded to add those flags onto older instructions.

There is a compatibility issue. kexc and kround can be added when they are needed leaving regular behaviour for cases where these flags are not specified, they also have property of having all fast-math flags unset by default. Combination of nexc and nrnd flags corresponds to current behaviour and all new code that wants folding will need to include these flags explicitly. Or is this what you mean by saying Older bitcode would have to be auto-upgraded to add those flags onto older instructions.?

@majnemer, I guess you were talking about AutoUpgrade.cpp rather than the issue I mentioned. I don't see a good way of handling this change, existing .ll files with FP instructions will change their meaning if not updated to include nexc and nrnd. I think such change isn't worth it. Is it? Or just keep kexc and kround (maybe rename it to krnd then)?

mehdi_amini edited edge metadata.Nov 25 2015, 9:38 AM

We don't provide any guarantee for .ll compatibility across version of llvm (best effort).
So it is only when reading a .bc that does not contain the flags that you need to set the default value correctly.

In D14067#296684, @joker.eph wrote:

We don't provide any guarantee for .ll compatibility across version of llvm (best effort).
So it is only when reading a .bc that does not contain the flags that you need to set the default value correctly.

Thank you, Mehdi.

Something that also probably needs to be thought about is what will be the default behavior for clang and how to control it?

Something that also probably needs to be thought about is what will be the default behavior for clang and how to control it?

The default behavior should be assume-default-rounding and dont-care-about-flags. These should be controlled jointly by #pragma STDC FENV_ACCESS [ON|OFF], and possibly individually by some other means.

Sounds reasonable to me, but it is unfortunate that the "default" textual IR will be clobbered with these two flags everywhere.

sdmitrouk updated this revision to Diff 41910.Dec 4 2015, 11:58 AM
sdmitrouk edited edge metadata.

Inverted meaning of flags, updated text of comments.

mehdi_amini added inline comments.Dec 4 2015, 12:09 PM
lib/Bitcode/Reader/BitcodeReader.cpp
4144

Could we avoid the code duplication by not doing it an else block here?

mehdi_amini accepted this revision.Dec 4 2015, 12:14 PM
mehdi_amini edited edge metadata.

LGTM, thanks.

This revision is now accepted and ready to land.Dec 4 2015, 12:14 PM

(but please try to remove the code duplication before committing if possible)

sdmitrouk updated this revision to Diff 42086.Dec 7 2015, 11:42 AM
sdmitrouk edited edge metadata.

Removed code duplication and moved several tests from patch#4 here. Will wait until patch#4 is accepted before committing, otherwise new flags will become visible in IR, but won't be used by any code, which can confuse.