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

fix bugzilla Issue 24819 - Optimizer changes result of float calculat… #17023

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

WalterBright
Copy link
Member

…ions on 32-bit

Thanks to @dkorpel for zeroing in on the problem.

@WalterBright WalterBright added Easy Review Backend glue code, optimizer, code generation labels Oct 24, 2024
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
24819 major Optimizer changes result of float calculations on 32-bit

Testing this PR locally

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

dub run digger -- build "master + dmd#17023"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the test case have -O as an argument?

Also this is a "major" issue, did you want to target stable for it?

@WalterBright
Copy link
Member Author

Shouldn't the test case have -O as an argument?

I thought it was good to test every combination.

Also this is a "major" issue, did you want to target stable for it?

@dkorpel is working with a customer on this, so I'll let him make that call. (It's been there a very long time.)

@thewilsonator
Copy link
Contributor

I thought it was good to test every combination.

Isn't that what permute args is for?

so I'll let him make that call.

Fair

@dkorpel
Copy link
Contributor

dkorpel commented Oct 24, 2024

Isn't that what permute args is for?

The default permute set is (-inline -release -g -O), so -O doesn't need to be added manually.

Copy link
Contributor

@dkorpel dkorpel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Did you find out why the sqrt instruction disappears with the x ^ signbit optimization? I hope there's no other latent bugs with integer optimizations on floating point math.

@thewilsonator
Copy link
Contributor

The default permute set is (-inline -release -g -O), so -O doesn't need to be added manually.

Yes but there is no //PERMUTE_ARGS directive(or however it is spelled) in the test file.

@veelo
Copy link
Contributor

veelo commented Oct 24, 2024

Many thanks for working on this, guys! I know we are probably the last bunch that need x87 to work correctly.

@dkorpel
Copy link
Contributor

dkorpel commented Oct 24, 2024

Yes but there is no //PERMUTE_ARGS directive(or however it is spelled) in the test file.

With default I mean that even without any PERMUTE_ARGS directive, it's testing both -O and unoptimized amongst other things.

@dkorpel dkorpel merged commit 88d1e8f into dlang:master Oct 24, 2024
41 checks passed
@WalterBright
Copy link
Member Author

did you find out ...

No, I just went with the elem tree was malformed.

@WalterBright
Copy link
Member Author

That optimization was a relic of the old no-x87 CPUs.

@WalterBright WalterBright deleted the t24819 branch October 24, 2024 15:53
@veelo
Copy link
Contributor

veelo commented Oct 24, 2024

This fix is important to us. Does it help if I make a PR to the stable branch with a cherry pick of this commit, or does that only add to your workload, @ibuclaw? Is master going to be merged into stable before the next beta release? (Edit: there is another change in master that will benefit us as well: #16780).

It would be good to have an approximate ETA of the next release. If that is months from now, we would have to decide between two unpopular options: 1. to use the nightly build, putting us on master, or 2. build our own compiler, possibly missing out on PGO. In both cases we mess up our system for reproducible builds (which relies on official version tags and the release archive).

@thewilsonator
Copy link
Contributor

Is master going to be merged into stable before the next beta release?

No. If you want them in the next release, please cherrypick this and #16780 onto stable
I don't think exact duplicate commits cause merge conflicts, but if you want to, you could always rebase master on top of stable after you are done.

veelo pushed a commit to veelo/dmd that referenced this pull request Oct 24, 2024
dlang-bot pushed a commit that referenced this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend glue code, optimizer, code generation Bug Fix Easy Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants