Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.math: change tests for rounding #7109

Merged
merged 1 commit into from
Jul 16, 2019
Merged

Conversation

WalterBright
Copy link
Member

Changing floating point rounding modes, because it happens off to the side, interferes with optimizing. In particular, common subexpressions may not be common if the rounding mode is changed.

Solutions are:

  1. disable CSEs for floating point (what dmd used to do). Not doing CSEs for floating point is kinda sad.
  2. make a compiler intrinsic for changing the rounding mode, so the optimizer knows about it, this would be a fair amount of rewrite both for the optimizer and std.math.
  3. change the code that mucks with the rounding mode so it won't interfere with optimization

This does option (3).

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7109"

@WalterBright
Copy link
Member Author

Option (2) remains a problem even with intrinsics, because if the intrinsic is wrapped in a function call, the optimizer still won't know it happened.

@WalterBright
Copy link
Member Author

This problem was exposed by dlang/dmd#10177 which improves CSE computation for DMD, and is blocking it.

@PetarKirov
Copy link
Member

PetarKirov commented Jul 15, 2019

Option (2) sounds like the best one in the long-term. Obviously you prefer a short-term solution which doesn't block your work, but I'm not sure about the conferences of (3). What concerns me is that a change in dmd's optimizer necessitates a change in user's code and I am not sure I fully understand the consequences. Does it mean that all code that changes the FP rounding mode inside one function body will be broken with the new optimizations? If not, then why this change is necessary, after all it is changing the optimization of unittest code (which doesn't seem actually important, enough so to block other work)?

@WalterBright
Copy link
Member Author

Does it mean that all code that changes the FP rounding mode inside one function body will be broken with the new optimizations?

It may be broken already. Optimizations that involve constant folding will be done in the compiler with round to nearest, which won't do the same thing at runtime if the rounding mode is set differently. Basically, you're going to have to know what you're doing when manipulating the rounding mode. Fortunately, few people ever do it.

@WalterBright
Copy link
Member Author

And as I noted, a compiler intrinsic probably won't be effective anyway.

@WalterBright
Copy link
Member Author

The buildkite weka error seems unrelated.

@WalterBright WalterBright force-pushed the rounding branch 2 times, most recently from b4cad31 to cb9e5fc Compare July 16, 2019 02:53
@dlang-bot dlang-bot merged commit 4cfe9db into dlang:master Jul 16, 2019
@WalterBright WalterBright deleted the rounding branch July 17, 2019 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants