Fix issue 20089: Handle extra case in fixresult_complex87 by marler8997 · Pull Request #10247 · dlang/dmd

marler8997 · 2019-07-30T23:19:31Z

Fix issue 20089, FPU stack not cleaned up properly

Note that this only occurs on windows 64-bit. It doesn't happen on windows 32 bit because it only occurs when the compiler tries to fit a cfloat into the RCX register. With help from @rainers, we added a new if block to handle the case when a cfloat needs to be copied to the one of the R*X register.

dlang-bot · 2019-07-30T23:19:33Z

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20089	major	FPU stack not cleaned up properly

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

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 + dmd#10247"

src/dmd/backend/cg87.d

rainers · 2019-07-31T06:07:34Z

I recently noticed this too: https://issues.dlang.org/show_bug.cgi?id=20089
I think the test would also fail an assertion with non-zero inputs.

rainers · 2019-07-31T14:10:55Z

src/dmd/backend/cg87.d

+        pop87();
+        cdb.genfltreg(ESC(MFfloat,1),BX,0);      // FSTP floatreg
+        genfwait(cdb);
+        cdb.genfltreg(LOD, CX, 0);              // MOV EAX,floatreg


I guess that cfloat is considered a struct, so it is passed in integer registers. But it fits into RCX, so I'd expect that one of the floats has to be packed into the upper half.
BTW: the comments show other registers.

Yeah that's kinda what I gathered from the code. However, I don't really understand exactly how this code works or how to move the value into the upper part of the register.

Based on the function comment, we need to generate code to move the value from retregs to *pretregs. In this case, retregs is in ST01, but I'm not sure what ST01 means. From wikipedia It looks like there are 8 stack registers (ST1 - ST7) but what is ST01? And I don't see where in this code block the ST01 register is being referenced. I just wrote this code block based on one of the other cases that is moving the value from ST01. I see the comment floatreg so maybe that's what ST01 is?

In any case, this appears to fix test18772 (or at least workaround it), I even added some values in test18772.d to make sure the real part of the float still works. However, I'm using small values so the upper part may be getting lost.

From wikipedia It looks like there are 8 stack registers (ST1 - ST7) but what is ST01?

There are registers ST0-ST7, where ST0 is the one on top of the stack, ST1 is the one below, and so on. mST01 is probably the pair of ST0 and ST1 used for complex numbers.

test/runnable/test18772.d

marler8997 · 2019-07-31T22:09:18Z

The PR as is fixes the test, though, the solution is not fully implemented. The commented line I included in the test will pass once the full solution is implemented.

I would be fine with integrating this partial solution as is and let someone more familiar with x86_64 assembly and DMD code generation to finish the solution.

The last part is that the imaginary part of the complex number is getting zero'd in the second MOV instruction to ECX. The first MOV instruction moves the imaginary part to ECX, then I shifted it left by 32 bits, but then the second mov to the lower 32 bits also sets the upper 32 bits to 0. Not sure the best way to solve this one. But since this fixes the test, the PR as is unblocks the PR that moves the build to D and I assume is re-enabling the asserts.

rainers · 2019-07-31T22:23:04Z

The last part is that the imaginary part of the complex number is getting zero'd in the second MOV instruction to ECX. The first MOV instruction moves the imaginary part to ECX, then I shifted it left by 32 bits, but then the second mov to the lower 32 bits also sets the upper 32 bits to 0. Not sure the best way to solve this one.

Yes, mov ECX,... clears the upper 32 bits of RCX. You have to move the float to another register, then or both together.

rainers · 2019-07-31T22:33:06Z

I would be fine with integrating this partial solution as is and let someone more familiar with x86_64 assembly and DMD code generation to finish the solution.

Maybe it might be better to disable the test until a general fix is found (your patch is a good starting point). I did so in #10231 not expecting a solution soon.

marler8997 · 2019-07-31T22:35:47Z

Yes, mov ECX,... clears the upper 32 bits of RCX. You have to move the float to another register, then or both together.

That was what I tried next but I couldn't figure out how to generate an OR instruction that worked with the 64-bit registers. The codegen api is pretty difficult to figure out.

rainers · 2019-08-01T07:41:07Z

@marler8997 I pushed a version to your branch that should fix the imaginary part (instead of or-ing the results, it just places the two values to consecutive memory locations). It also covers other register targets.
In other situations XMM registers are used instead, but that seems to be broken, too.

marler8997 · 2019-08-01T07:42:24Z

@rainers very nice, thanks alot for doing this!

rainers · 2019-08-02T06:46:18Z

I've added a fix for the failing test case, but I'm not so confident this time.

thewilsonator · 2019-08-20T11:42:53Z

Ping, this good to go?

…t, allow returning in other registers

… function

marler8997 · 2019-08-20T12:53:36Z

I've fixed the merge conflicts, I would say it's ready, we'll see what @rainers says.

rainers · 2019-08-20T20:30:52Z

Fine with me.

marler8997 force-pushed the 8087stack branch from 0cf8c09 to c4594d5 Compare July 31, 2019 04:34

marler8997 changed the title ~~Save all values in the 8087 stack~~ Handle extra case in fixresult_complex87 Jul 31, 2019

marler8997 commented Jul 31, 2019

View reviewed changes

src/dmd/backend/cg87.d Outdated Show resolved Hide resolved

marler8997 force-pushed the 8087stack branch 2 times, most recently from e65cc31 to f5b2800 Compare July 31, 2019 05:37

rainers reviewed Jul 31, 2019

View reviewed changes

marler8997 force-pushed the 8087stack branch 3 times, most recently from 6aeea9f to 4b28d85 Compare July 31, 2019 16:13

dlang-bot added the Severity:Bug Fix label Jul 31, 2019

marler8997 changed the title ~~Handle extra case in fixresult_complex87~~ Fix issue 20089: Handle extra case in fixresult_complex87 Jul 31, 2019

rainers reviewed Jul 31, 2019

View reviewed changes

test/runnable/test18772.d Outdated Show resolved Hide resolved

marler8997 force-pushed the 8087stack branch 3 times, most recently from 2fa2686 to 2dd6ffe Compare July 31, 2019 20:06

marler8997 mentioned this pull request Aug 1, 2019

Second round of makefile/build.d consolidation #10212

Merged

wilzbach added the Review:Blocking Other Work review and pulling should be a priority label Aug 11, 2019

marler8997 and others added 3 commits August 20, 2019 06:51

Fix issue 20089: Handle extra case in fixresult_complex87

3e2c68b

fixresult_complex87 pass through interger register: fix imaginary par…

c179a4f

…t, allow returning in other registers

avoid xmm calculation when passing result of cfloat multiplication to…

c4b00f8

… function

marler8997 force-pushed the 8087stack branch from c23be6f to c4b00f8 Compare August 20, 2019 12:52

thewilsonator merged commit aa94684 into dlang:master Aug 21, 2019

marler8997 deleted the 8087stack branch August 21, 2019 04:11

Uh oh!

Conversation

marler8997 commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

Uh oh!

Uh oh!

rainers commented Jul 31, 2019

Uh oh!

rainers Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

marler8997 Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marler8997 commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Jul 31, 2019

Uh oh!

rainers commented Jul 31, 2019

Uh oh!

marler8997 commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Aug 1, 2019

Uh oh!

marler8997 commented Aug 1, 2019

Uh oh!

rainers commented Aug 2, 2019

Uh oh!

thewilsonator commented Aug 20, 2019

Uh oh!

marler8997 commented Aug 20, 2019

Uh oh!

rainers commented Aug 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marler8997 commented Jul 30, 2019 •

edited

Loading

dlang-bot commented Jul 30, 2019 •

edited

Loading

marler8997 Jul 31, 2019 •

edited

Loading

marler8997 commented Jul 31, 2019 •

edited

Loading

marler8997 commented Jul 31, 2019 •

edited

Loading