Re: Ping: [patch] -m128bit-long-double asserts

[Stuart Hastings <stuart at apple dot com>]

On Mar 31, 2005, at 3:30 PM, Geoffrey Keating wrote:

> Hi Stuart,
>
> If you look at subreg_offset_representable_p, you'll see that indeed
> it does not work properly if the assert does not hold.  For instance,
> suppose
>
> ymode == SImode  (size 4)
> xmode == XCmode  (size 32)
> nregs_xmode == 6
> nregs_ymode == 1
> offset == 3 + 12
>
> so
>
> y_offset = 3
> mode_multiple = 8
> nregs_multiple = 6
>
> You'll see that in fact this is not representable, because there is no
> register which actually holds that word, but

[if Stuart deletes the troublesome assert]

> the routine will
> incorrectly return true, saying that it is representable.  I expect
> that subreg_regno_offset will fail for many cases where the offset
> actually is representable.

(Forgive my insertion; your statement seemed a bit ambiguous to me, so 
I inferred the meaning shown above.)

If I understood what you suggested, well, O.K., but it would assert the 
same way if invoked with a /valid/ offset.  The failing assert fires 
due to dis-alignment of modes and hard register multiples, and is not 
affected by the value of the 'offset' parameter.

> Thus, in order to remove that 'assert', you'd have to modify both
> subreg_offset_representable_p and subreg_regno_offset to deal with
> such cases correctly.  There are probably other bugs you'd find too.

O.K., the assert can stay.

> This takes me to the second patch, which seems correct as far as it
> goes, but needs the corresponding change to XCmode, to prevent exactly
> the case above.

O.K., but I don't understand how this fixes your (contrived, but 
excellent) example.  I tried the GDB equivalent of "(subreg:SI (reg:XC) 
15)", and it asserts  on the previous line ("gcc_assert ((offset % 
GET_MODE_SIZE (ymode)) == 0);").

I'd be grateful if you could explain why it's better to pretend that an 
80-bit XFmode datum that is allocated 128 bits of memory should be 
accorded four hard SImode registers.  There are really only three 
(2.5?) SImode datums there; telling subreg_offset_representable_p 
there's a fourth seems like misrepresentation to me (?).

I don't know where you came up with your XCmode example; perhaps you 
noticed the XCmode datum in the testcase.  That's good, but the 
original failure was due to something like (recalling from weak memory) 
"(subreg:SI (reg:XF) 12)".  The XCmode in the testcase seems to be 
necessary to recreate the problem, but only distantly; when the assert 
fires, XCmode is not evident.

To insure we're talking about the same patch, here's what I thought you 
suggested:

Attachment: i386.h.diffs.txt
Description: Text document

2005-04-01  Stuart Hastings  <stuart@apple.com>

        * config/i386/i386.h(HARD_REGNO_NREGS): Aware of 
-m128bit-long-double.

(This version hasn't been bootstrapped yet...)

Messing about with the debugger, I validated your concerns about 
XCmode; ("(subreg:SI (reg:XC) 12)") asserted too.  Adding the change 
you suggested (patch above), XCmode no longer asserts.  Using the 
debugger commandline, I noticed it correctly asserts if you ask for an 
odd offset (e.g. "(subreg:SI (reg:XF) 5)"), but it's happy with any 
offset that's a multiple of the outer mode size (e.g. "(subreg:SI 
(reg:XF) 1280)").  I hope this makes sense to somebody.

I have yet to make a thorough study of this routine, but it looks to me 
like it was intended to filter out crazy subregs created in the unholy 
crucible :-) of the reload phase, tolerate big-endian and little-endian 
targets, and as a place to hang some sanity checking (all those 
asserts).  It returned TRUE for most of the combinations I tried; I was 
able to get FALSE from it by asking for very strange things like 
"(subreg:HI (reg:XF) 2)".

I need to give up for the night so I can deal with my children in the 
morning.  Let me know if you like the patch above; even it doesn't 
solve everything, it's arguably better than ICE-ing on the testcase as 
we do currently.

stuart