Floating point and xmm codegen #769

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
3 participants
Member

yebblies commented Feb 26, 2012

Starting with 4155, every time I fixed one bug I ran into another while testing it.

4155 - jmpopcode assumes that <80bit floating point types are converted to bool by moving them into integer registers and doubling, but this is not the case when they're returned from a function on the floating point stack or in xmm registers.

7581 - fixresult_complex87 treats cfloat and cdouble exactly the same. This means a trying to return a cfloat in an xmm reg which is on the floating point stack results the value getting written to memory as a double, read into xmm as a double, then returned to code expecting the xmm regs to hold a float.

7591 - The backend constfolding code for complex ==/!= incorrectly inverts the result if either operand contains nans. Using the built-in comparison generates the correct result.

7592 - e2ir contains a typo which makes it use the wrong op for casting from ireal -> ifloat via idouble

7593 - cdcnvt takes the wrong path when converting cfloat to cdouble, because the check to see if dealing with complex types is after the check for using xmm regs

7594 - Code generation for real+imaginary,imaginary+real,real-imaginary and
imaginary-real is done by xmmorth, which implements it the same as real+real,
resulting in very wrong code.

http://d.puremagic.com/issues/show_bug.cgi?id=4155
http://d.puremagic.com/issues/show_bug.cgi?id=7581
http://d.puremagic.com/issues/show_bug.cgi?id=7591
http://d.puremagic.com/issues/show_bug.cgi?id=7592
http://d.puremagic.com/issues/show_bug.cgi?id=7593
http://d.puremagic.com/issues/show_bug.cgi?id=7594

Member

yebblies commented Feb 26, 2012

@WalterBright I'm not 100% sure about the changes in cod3 and cod4 - especially cod3.

Owner

WalterBright commented Feb 26, 2012

Once again, please for commit messages, use:

"fix Issue NNNN"

- i ^= (int)((e1->EV.Vcfloat.re == e2->EV.Vcfloat.re) &&
- (e1->EV.Vcfloat.im == e2->EV.Vcfloat.im));
+ i ^= (int)((e1->EV.Vcfloat.re == e2->EV.Vcfloat.re) &&
+ (e1->EV.Vcfloat.im == e2->EV.Vcfloat.im));
@WalterBright

WalterBright Feb 26, 2012

Owner

Why this change? Also, some compilers do not handle nan, so using the builtin == does not always work.

@yebblies

yebblies Feb 27, 2012

Member

Because it's wrong.

Take (OPeqeq (OPconst TYcfloat Nan NaN) (OPconst TYcfloat 0.f 0.f)) for example.

i is 0, then because e1 contains nans, i is flipped and somehow cfloat.nan == (1.0f + 1.0fi) is true?

I understand that the special case code exists because of past problems using the built-in comparisons, but if you look a few lines down the default case is handled using the default comparison... so it seems it is working everywhere it has been tested.

In short I followed the lead of the existing working code.

@@ -1353,20 +1353,21 @@ int jmpopcode(elem *e)
e = e->E2; /* right operand determines it */
op = e->Eoper;
- if (e->Ecount != e->Ecomsub) // comsubs just get Z bit set
- return JNE;
@WalterBright

WalterBright Feb 26, 2012

Owner

This looks really wrong. What's the test case for it?

@yebblies

yebblies Feb 27, 2012

Member

The 4155 test case had a problem with it. As I said, I'm not 100% sure this is correct but from what I could tell this if block was chosen while the the expression was being evaluated as normal, and needed the extra jpe to handle the nan case. It seemed as if the comsub code does not apply to floating point values, at least not in this case.

Moving this condition back to here and running the test cases should show how this fails, I'll re-reduce it for you later but I'm not on my dev machine at the moment.

@WalterBright

WalterBright Feb 27, 2012

Owner

It looks suspicious to me because floating point expressions are not common subexpressioned.

@yebblies

yebblies Feb 27, 2012

Member

Ah. So maybe the bug is that it gets detected as a comsub in the first place? I'll try and find where/why it happens, but my guess is it has something to do with the way movxmmconst loads values through the gp registers or something like that. Or possibly there is an exception somewhere for ST but not XMM.
I'll try and do it tonight if I have time, but moving the line back and running the test cases should be enough to reproduce it if you don't want to wait.

Member

yebblies commented Feb 27, 2012

Yeah, the only reason I hadn't done the commit messages is because I created the bug reports after I fixed the bugs, and wanted to be sure it passed the test suite on all platforms before I refactored/cleaned up. I'll fix it up tonight.

Owner

WalterBright commented Mar 10, 2012

Blast. How do I just pull some of the changes? I tried:

git pull https://github.com/yebblies/dmd.git d847c1c

and it failed with:

fatal: Couldn't find remote ref d847c1c

Member

yebblies commented Mar 10, 2012

You should be able to do it though the fork queue on github. Go to dmd repository -> network tab -> fork queue -> find the commit you want and apply it. But feel free to just copy paste for this pull request if you want, I probably won't have time to fix it up for a couple of weeks unfortunately.

Owner

WalterBright commented Mar 10, 2012

I've never been able to get past this message from the fork queue:

showing the first 200 of 879 pending commits

The most recent commits are in the 679.

Owner

braddr commented Mar 10, 2012

It's been a while since I've done this, but try:

// this will pull the changes into your repo but will NOT merge it into anything
git fetch https://github.com/yebblies/dmd.git complexxmm

// apply a single commit into your current tree (and commits, add -n to not commit)
git cherry-pick d847c1c

Owner

WalterBright commented Mar 10, 2012

Thanks, will try that one.

- c2 = genfltreg(c2, 0xF20F11, XMM1-XMM0, 0); // MOV floatreg, XMM1
- genfltreg(c2, 0xDD, 0, 0); // FLD double ptr floatreg
+ c2 = genfltreg(c2, xop, XMM1-XMM0, 0); // STOS(SD) floatreg, XMM1
+ genfltreg(c2, fop, 0, 0); // FLD float/double ptr floatreg
@WalterBright

WalterBright Mar 17, 2012

Owner

Folding in just these changes to cg87.c results in:

core.exception.AssertError@std/format.d(1272): expected = 1+1i, result = 1+0i

for 64 bit Linux only.

@yebblies

yebblies Mar 18, 2012

Member

Yes, it exposes another bug. I think 7581, and this commit

IIRC it doesn't move the imaginary part correctly, because the non-complex path is taken. Hence it's zero (sometimes). The addition/subtraction problems may also be related.

@WalterBright

WalterBright Apr 25, 2012

Owner

Yes, that commit fixes it.

+ return val;
+}
+
+void testb()
@yebblies

yebblies Mar 18, 2012

Member

@WalterBright This test case should reproduce the std.format bug.

WalterBright added a commit that referenced this pull request Mar 18, 2012

WalterBright added a commit that referenced this pull request Mar 18, 2012

This fixes the std.format failure. Thanks!

I incorporated lines 1362 and 1363.

Owner

WalterBright commented May 19, 2012

All the bugs listed have been resolved.

Member

yebblies commented May 20, 2012

There is still a bug in here (derived from 4155) but I haven't reduced it. Something to do with complex fp ops incorrectly getting picked up by cse then using the wrong jump opcodes.

Owner

WalterBright commented May 20, 2012

When you find it, please post a bugzilla entry for it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment