Bug 5455 – ICE(cgcod.c): Optimization (register allocation?) regression in DMD 1.065

Status
RESOLVED
Resolution
FIXED
Severity
regression
Priority
P2
Component
dmd
Product
D
Version
D2
Platform
x86
OS
Windows
Creation time
2011-01-14T18:51:00Z
Last change time
2015-06-09T05:11:52Z
Keywords
ice-on-valid-code
Assigned to
nobody
Creator
dlang-bugzilla

Attachments

IDFilenameSummaryContent-TypeSize
871Buggccod.zipPartially minimized test caseapplication/zip7807

Comments

Comment #0 by dlang-bugzilla — 2011-01-14T18:51:29Z
Created attachment 871 Partially minimized test case I've stumbled upon a rather weird regression, which was introduced in DMD 1.065: when compiling specific code with -O -release -inline, it ICEs with "Internal error: ..\ztc\cgcod.c 1649". The test case seems to be quite resistant to "minimization", and even moving functions from one module to another seems to make the bug vanish. I've reduced the test case to four modules, and somewhat minimized the first two, but I'm willing to give up for now at this point. If any compiler developer decides that minimizing the test case is worth it, I'll give it another shot as soon as I get over my cold. As I went to post this, I noticed that I was still using DMD 1.065. However, DMD 1.066 outright crashes. To reproduce, compile the attached files with: dmd -O -release -inline Common.d Data.d Gzip.d Zlib.d
Comment #1 by clugdbug — 2011-01-17T00:22:27Z
This doesn't fail in the D1 version in svn. This doesn't ICE on svn 852 "reduce reliance on varargs" but did ICE on svn 851 and earlier. Here's a slightly reduced test case which ICEs on the version of D2 in svn. Still requires -O -inline -release. ---- final class DataWrapper { void* dat; void[] contents() { return dat[0..1]; } } class Data { private: DataWrapper wrapper; size_t start, end; public: this(void[] data) {} size_t length() { return end - start; } } Data daz(Data srcbuf) { throw new Exception("xxx"); } uint crc32(void[] data) { int w; foreach(v;cast(ubyte[])data) { ++w; } return 0; } class HttpResponse { Data zug; void setContent(Data data, string[] supported) { foreach(method;supported) switch(method) { default: uint[] footer = [crc32(data.wrapper ? data.wrapper.contents[data.start..data.end] : null ), data.end - data.start]; zug = daz(data); return; } } } ----
Comment #2 by clugdbug — 2011-01-17T11:44:32Z
This is amazing. I have never before seen such an irreducible test case. Here's the best I've been able to do. ----- void thrw(Data *s) { throw new Exception("xxx"); } struct Data { Rapper *w; uint n, m; } struct Rapper { ubyte * dat; ubyte[] con() { return dat[0..1]; } } uint jaz(ubyte[] data) { return data.length; } struct Resp { void set(Data *data, string[] soup) { switch(soup[0]) { default: } uint[] f = [jaz(data.w ? data.w.con[data.n ..data.m] : null), data.m - data.n]; thrw(data); } }
Comment #3 by clugdbug — 2011-01-18T12:44:11Z
I think there's something wrong in cgcod.c, allocreg(). Might be good to have an assert on line 1920: r &= mLSW; /* see if there's an LSW also */ if (r) lsreg = findreg(r); else if (lsreg == -1) /* if don't have LSW yet */ { retregs &= mLSW; + assert(retregs); goto L3; } } This fails. What happens is, we've found a register for the MSW. But we got called with no available LSW registers!
Comment #4 by clugdbug — 2011-01-24T14:03:21Z
The regression was introduced in git revision 0eb915bf22045b3c34f23e63859bf8361b2d5a7a, 2010-10-24 with the helpful commit comment "more 64 bit stuff". That commit was TYint-> TYsizet changes, so it shouldn't have had any effect at all on the generated code for 32 bits.
Comment #5 by clugdbug — 2011-01-25T00:27:34Z
The bug was triggered by this change to SliceExp::toElem() (now line 4426): --- elem *eptr = array_toPtr(e1->type, e); - elem *elength = el_bin(OPmin, TYint, eupr, elwr2); + elem *elength = el_bin(OPmin, TYsize_t, eupr, elwr2); eptr = el_bin(OPadd, TYnptr, eptr, el_bin(OPmul, TYsize_t, el_copytree(elwr2), el_long(TYsize_t, sz))); e = el_pair(TYdarray, elength, eptr); e = el_combine(elwr, e); e = el_combine(einit, e); --- Reverting this change removes the ICE. TYsize_t is a TYuint, rather than a TYint. Still, I don't understand why it makes any difference.
Comment #6 by clugdbug — 2011-01-25T13:07:25Z
I think what's happened, is that a minor change to support 64 bits has triggered a latent bug in the backend. Basically, cgreg_assign() in cgreg.c thinks there are enough free registers. But allocreg() in cgcod.c can't find one. Although it is called with two available registers, one of them is EBP, and EBP is disallowed in line 1910. So these two functions are inconsistent. I think the problem may be in creg_assign(). It thinks it has freed up a register (EBX) which is used for 'this'. But (if I've correctly understood what the code is doing) the same register seems to be used in a double-reg combo. Seems a bit similar to bug 4443. If I'm correct, then a solution to this bug would be to check if the register is also part of a double-reg variable, before marking it as freed-up.
Comment #7 by bugzilla — 2011-02-11T22:59:37Z