fix Issue 15861 - [REG 2.069] Wrong double-to-string conversion with -O

[WalterBright]

https://issues.dlang.org/show_bug.cgi?id=15861

[dlang-bot]

Fix	Bugzilla	Description
✓	15861	[REG 2.069] Wrong double-to-string conversion with -O

[yebblies]

That's not a helpful test case. Can't you reduce it now that you know which sub-part of the code it was in?

[WalterBright]

As will all most optimizer problems, they depend on a very specific path through the system, which is pretty hard to reduce. In this case, it'd likely take several hours work. Even then, such test cases tend to be fragile, as some other change somewhere else in the compiler will invalidate them.

This sort of thing is why "fuzz testers" are effective, but we don't have a fuzz tester. Wanna make one?

[yebblies]

As will all most optimizer problems, they depend on a very specific path through the system, which is pretty hard to reduce. In this case, it'd likely take several hours work. Even then, such test cases tend to be fragile, as some other change somewhere else in the compiler will invalidate them.

I asked because sometimes the problem construct is obvious once the bug has been found. That makes me think we're better off with no test case than a useless test case, or putting it in phobos instead. We shouldn't be adding more phobos-dependent test cases.

This sort of thing is why "fuzz testers" are effective, but we don't have a fuzz tester. Wanna make one?

I've made several, but none specifically for the optimizer. We really need the ability to create elem-to-elem test cases.

[WalterBright]

I put the test case in its own file, because it depends on phobos. I understand the value of a test suite that does not depend on Phobos, because code gen bugs causing Phobos to fail are hard to debug.

The optimization I removed was simply wrong. A double move cannot be replaced with two float moves. It's essentially putting random data into a float register, and since some bit patterns are not always supported, documented, or behave portably between FPUs, unpredictable things can happen. Such things are hard to reliably detect with a test case.

[yebblies]

My concern is that the only useful thing we'll be able to do with the test case in the future is delete it. It's unlikely to get reduced in the future, and will be very sensitive to changes in both dmd and phobos. I feel like it doesn't actually add any value, and we'd be better off not having it at all.

Anyway, the compiler change LGTM so if you're dead set on keeping the test case feel free to toggle auto-merge on.

[WalterBright]

Auto-merge toggled on

[9rnsr]

Why this PR is based on master? Does it mean this regression fix needs to be delayed until 2.072.0 release?

[CyberShadow]

@WalterBright @yebblies @9rnsr Here is a reduced test case:

void main()
{
    double val = 4286853117.;

    (){
        assert(val == 4286853117.);
    }();
}

Maybe you could include it too when you put this on stable.

Also feel free to ping me if you'd like something reduced or bisected :)

[WalterBright]

@CyberShadow thank you, this is awesome. I'll do a new PR to replace the test case with yours.