Bug 12164 – Function returning ptrdiff_t.min in 64-bit returning 0 when -O is set.
Status
RESOLVED
Resolution
FIXED
Severity
critical
Priority
P2
Component
dmd
Product
D
Version
D2
Platform
x86_64
OS
All
Creation time
2014-02-14T17:58:00Z
Last change time
2014-08-22T08:04:59Z
Keywords
pull, wrong-code
Assigned to
yebblies
Creator
opantm2+dbugs
Comments
Comment #0 by opantm2+dbugs — 2014-02-14T17:58:07Z
The title isn't very descriptive, but perhaps the sample is more useful. Reduced from std.variant.
Uncommenting the writeln, using -m32, commenting out the if(*rhsPA == *zis) return 0, or not passing in -O causes the problem to not occur. Otherwise compile the below with -O -m64 to have it occur.
Sample:
import std.stdio, std.exception, std.c.string;
struct Foo {
ubyte[32] store;
this(A value) {
memcpy(&store, &value, value.sizeof);
}
static ptrdiff_t compare(A* rhsPA, A* zis) {
if (*rhsPA == *zis)
return 0;
//writeln("Returning min");
return ptrdiff_t.min;
}
bool opEquals(Foo rhs) const {
auto zis = cast(A*)&store;
auto rhsPA = cast(A*)&rhs.store;
// Below prints 0 if previous writeln line is commented and -O is set.
// Otherwise prints -9223372036854775808 as expected.
writeln(compare(rhsPA, zis));
return compare(rhsPA, zis) == 0;
}
}
static struct A { int a; }
void main() {
enforce(Foo(A(3)) != Foo(A(4)));
}
Comment #1 by yebblies — 2014-02-15T08:05:08Z
Wrong code, yummy!
Comment #2 by opantm2+dbugs — 2014-02-16T00:02:09Z
Raised priority to critical as it's a difficult to notice or track down (since trying to observe it with something like a writeln or setting a global variable causes it to no longer occur) bug that will silently result in different behaviour only in release mode.
Comment #3 by yebblies — 2014-03-06T23:49:05Z
Reduced:
ptrdiff_t compare(A* rhsPA, A* zis)
{
if (*rhsPA == *zis)
return 0;
return ptrdiff_t.min;
}
struct A
{
int a;
}
void main()
{
auto a = A(3);
auto b = A(4);
assert(!compare(&a, &b));
}
The backend tried to emit OPmemcmp and screws it up somehow, probably missing a REX_W prefix or two.
Comment #4 by safety0ff.bugz — 2014-03-09T22:08:33Z
(In reply to comment #3)
> The backend tried to emit OPmemcmp and screws it up somehow, probably missing a
> REX_W prefix or two.
When I looked at the assembly on my end the memcmp looked fine.
The emitted code for returning ptrdiff.min or 0 depending on the memcmp result looks like it has more than one issue in it.
Comment #5 by github-bugzilla — 2014-03-15T10:21:55Z
(In reply to comment #4)
> (In reply to comment #3)
> > The backend tried to emit OPmemcmp and screws it up somehow, probably missing a
> > REX_W prefix or two.
>
> When I looked at the assembly on my end the memcmp looked fine.
> The emitted code for returning ptrdiff.min or 0 depending on the memcmp result
> looks like it has more than one issue in it.
If someone can identify exactly which instructions are wrong, and what they should be, I can probably fix it fairly easily. Otherwise it will have to wait, I don't have the energy to go digging through the assembly right now.
Comment #7 by safety0ff.bugz — 2014-03-31T07:33:59Z
(In reply to comment #6)
> If someone can identify exactly which instructions are wrong, and what they
> should be, I can probably fix it fairly easily. Otherwise it will have to
> wait, I don't have the energy to go digging through the assembly right now.
I don't think the solution is as simple as you think (it's not a simple REX issue):
Un-optimized (correct) assembly:
55 push RBP
48 8B EC mov RBP,RSP
48 83 EC 10 sub RSP,010h
48 B9 04 00 00 00 00 00 00 00 mov RCX,4
33 C0 xor EAX,EAX
F3 rep
A6 cmpsb
75 05 jne L1D
48 31 C0 xor RAX,RAX
C9 leave
C3 ret
L1D: 48 B8 00 00 00 00 00 00 00 80 mov RAX,08000000000000000h
C9 leave
C3 ret
Incorrect -O assembly:
55 push RBP
48 8B EC mov RBP,RSP
48 83 EC 10 sub RSP,010h
48 B9 04 00 00 00 00 00 00 00 mov RCX,4
33 C0 xor EAX,EAX
F3 rep
A6 cmpsb
74 05 je L1D
1B C0 sbb EAX,EAX
83 D8 FF sbb EAX,0FFFFFFFFFFFFFFFFh
L1D: 83 F8 01 cmp EAX,1
19 C0 sbb EAX,EAX
31 C0 xor EAX,EAX
48 8B E5 mov RSP,RBP
5D pop RBP
C3 ret
As you can see, it invariantly returns zero after doing the memcmp.
Comment #8 by blah38621 — 2014-03-31T07:49:58Z
One thing I am curious of in that generated code, why is it allocating 16 bytes on the stack when it doesn't use the stack? (sub RSP, 0x10) Also, I missing something here, but when did parameters start getting passed in ESI and EDI?
As a bonus question, I have to ask, why are we doing that comparison with cmpsb when the size of the values being compared is known to be a multiple of 4, in which case, shouldn't we be using cmpsd? (this is quite literally a 4x speed improvement, as both instructions have the same cycle count)
Comment #9 by blah38621 — 2014-03-31T08:49:18Z
(In reply to comment #8)
> One thing I am curious of in that generated code, why is it allocating 16 bytes
> on the stack when it doesn't use the stack? (sub RSP, 0x10) Also, I missing
> something here, but when did parameters start getting passed in ESI and EDI?
>
> As a bonus question, I have to ask, why are we doing that comparison with cmpsb
> when the size of the values being compared is known to be a multiple of 4, in
> which case, shouldn't we be using cmpsd? (this is quite literally a 4x speed
> improvement, as both instructions have the same cycle count)
I seem to have forgotten about cmpsq, which is available on x64, for the same cycle count, which would be 8x faster.
Comment #10 by yebblies — 2014-07-23T15:54:29Z
Found it!
https://github.com/D-Programming-Language/dmd/pull/3805
ptrdiff_t.min was being truncated to 32-bits == 0 while trying to optimize the ternary operator it built out of the compare function.
The xor eax, eax was done by the pinhole optimizer working off the already broken code.
Comment #11 by github-bugzilla — 2014-07-24T21:24:09Z