* [PATCH][GCC 7] Remove broken path in extract_bit_field_1
@ 2016-04-04 12:57 Richard Biener
2016-04-04 13:06 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2016-04-04 12:57 UTC (permalink / raw)
To: gcc-patches; +Cc: Jakub Jelinek, iant
r141430 as a fix for PR37870 added a memory fallback to
extract_bit_field_1 and another case punning through a larger integer
mode (as suggested by Ian). That path is broken as it happily creates
(set (subreg:XF (reg:TI ..)) (...))
on i?86 when we optimize the added testcase
unsigned int
bar (long double x)
{
union { struct { char a[8]; unsigned int b:7; } c; long double d; } u;
u.d = x;
return u.c.b;
}
on GIMPLE to
bar (long double x)
{
unsigned int _3;
<unnamed-unsigned:7> _4;
;; basic block 2, loop depth 0
;; pred: ENTRY
_4 = BIT_FIELD_REF <x_2(D), 7, 64>;
_3 = (unsigned int) _4;
return _3;
Bootstrapped and tested on x86_64-unknown-linux-gnu (also with -m32)
and the code replaced with a gcc_unreachable ().
I don't see how a subreg can be ever valid as it will be necessarily
converting to a non-integer mode of different size (otherwise
int_mode_for_mode would have returned non-BLKmode). I can't think
of a condition that would need to be satisfied where the subreg
would be valid either.
Ok for GCC 7? (it'll be done as part of said GIMPLE optimization).
The testcase gcc.target/i386/pr37870.c will already ICE with that
patch, so no additional testcase.
Thanks,
Richard.
2016-04-04 Richard Biener <rguenther@suse.de>
PR middle-end/37870
* expmed.c (extract_bit_field_1): Remove broken case
using a wider MODE_INT mode.
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c (revision 234708)
+++ gcc/expmed.c (working copy)
@@ -1647,17 +1647,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
if (GET_CODE (op0) == SUBREG)
op0 = force_reg (imode, op0);
}
- else if (REG_P (op0))
- {
- rtx reg, subreg;
- imode = smallest_mode_for_size (GET_MODE_BITSIZE (GET_MODE (op0)),
- MODE_INT);
- reg = gen_reg_rtx (imode);
- subreg = gen_lowpart_SUBREG (GET_MODE (op0), reg);
- emit_move_insn (subreg, op0);
- op0 = reg;
- bitnum += SUBREG_BYTE (subreg) * BITS_PER_UNIT;
- }
else
{
HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1
2016-04-04 12:57 [PATCH][GCC 7] Remove broken path in extract_bit_field_1 Richard Biener
@ 2016-04-04 13:06 ` Jakub Jelinek
2016-04-04 13:10 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2016-04-04 13:06 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, iant
On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> The testcase gcc.target/i386/pr37870.c will already ICE with that
> patch, so no additional testcase.
In theory you could validate_subreg first and use that code if validation
went ok, otherwise go through memory.
But I admit I don't have anything in particular in mind where it would
trigger this code and the subreg would successfully validate.
> 2016-04-04 Richard Biener <rguenther@suse.de>
>
> PR middle-end/37870
> * expmed.c (extract_bit_field_1): Remove broken case
> using a wider MODE_INT mode.
>
> Index: gcc/expmed.c
> ===================================================================
> --- gcc/expmed.c (revision 234708)
> +++ gcc/expmed.c (working copy)
> @@ -1647,17 +1647,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
> if (GET_CODE (op0) == SUBREG)
> op0 = force_reg (imode, op0);
> }
> - else if (REG_P (op0))
> - {
> - rtx reg, subreg;
> - imode = smallest_mode_for_size (GET_MODE_BITSIZE (GET_MODE (op0)),
> - MODE_INT);
> - reg = gen_reg_rtx (imode);
> - subreg = gen_lowpart_SUBREG (GET_MODE (op0), reg);
> - emit_move_insn (subreg, op0);
> - op0 = reg;
> - bitnum += SUBREG_BYTE (subreg) * BITS_PER_UNIT;
> - }
> else
> {
> HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1
2016-04-04 13:06 ` Jakub Jelinek
@ 2016-04-04 13:10 ` Richard Biener
2016-04-04 13:24 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2016-04-04 13:10 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, iant
On Mon, 4 Apr 2016, Jakub Jelinek wrote:
> On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> > The testcase gcc.target/i386/pr37870.c will already ICE with that
> > patch, so no additional testcase.
>
> In theory you could validate_subreg first and use that code if validation
> went ok, otherwise go through memory.
> But I admit I don't have anything in particular in mind where it would
> trigger this code and the subreg would successfully validate.
Not sure if it would help as that has
/* ??? Similarly, e.g. with (subreg:DF (reg:TI)). Though
store_bit_field
is the culprit here, and not the backends. */
else if (osize >= UNITS_PER_WORD && isize >= osize)
;
and thus we'd return true anyway for (subreg:XF (reg:TI) 0)
Richard.
> > 2016-04-04 Richard Biener <rguenther@suse.de>
> >
> > PR middle-end/37870
> > * expmed.c (extract_bit_field_1): Remove broken case
> > using a wider MODE_INT mode.
> >
> > Index: gcc/expmed.c
> > ===================================================================
> > --- gcc/expmed.c (revision 234708)
> > +++ gcc/expmed.c (working copy)
> > @@ -1647,17 +1647,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
> > if (GET_CODE (op0) == SUBREG)
> > op0 = force_reg (imode, op0);
> > }
> > - else if (REG_P (op0))
> > - {
> > - rtx reg, subreg;
> > - imode = smallest_mode_for_size (GET_MODE_BITSIZE (GET_MODE (op0)),
> > - MODE_INT);
> > - reg = gen_reg_rtx (imode);
> > - subreg = gen_lowpart_SUBREG (GET_MODE (op0), reg);
> > - emit_move_insn (subreg, op0);
> > - op0 = reg;
> > - bitnum += SUBREG_BYTE (subreg) * BITS_PER_UNIT;
> > - }
> > else
> > {
> > HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1
2016-04-04 13:10 ` Richard Biener
@ 2016-04-04 13:24 ` Jakub Jelinek
2016-04-04 13:27 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2016-04-04 13:24 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, iant
On Mon, Apr 04, 2016 at 03:10:34PM +0200, Richard Biener wrote:
> On Mon, 4 Apr 2016, Jakub Jelinek wrote:
>
> > On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> > > The testcase gcc.target/i386/pr37870.c will already ICE with that
> > > patch, so no additional testcase.
> >
> > In theory you could validate_subreg first and use that code if validation
> > went ok, otherwise go through memory.
> > But I admit I don't have anything in particular in mind where it would
> > trigger this code and the subreg would successfully validate.
>
> Not sure if it would help as that has
>
> /* ??? Similarly, e.g. with (subreg:DF (reg:TI)). Though
> store_bit_field
> is the culprit here, and not the backends. */
> else if (osize >= UNITS_PER_WORD && isize >= osize)
> ;
>
> and thus we'd return true anyway for (subreg:XF (reg:TI) 0)
If XFmode subreg of TImode reg passes validation, where does it ICE then?
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1
2016-04-04 13:24 ` Jakub Jelinek
@ 2016-04-04 13:27 ` Richard Biener
2016-04-04 13:33 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2016-04-04 13:27 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, iant
On Mon, 4 Apr 2016, Jakub Jelinek wrote:
> On Mon, Apr 04, 2016 at 03:10:34PM +0200, Richard Biener wrote:
> > On Mon, 4 Apr 2016, Jakub Jelinek wrote:
> >
> > > On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> > > > The testcase gcc.target/i386/pr37870.c will already ICE with that
> > > > patch, so no additional testcase.
> > >
> > > In theory you could validate_subreg first and use that code if validation
> > > went ok, otherwise go through memory.
> > > But I admit I don't have anything in particular in mind where it would
> > > trigger this code and the subreg would successfully validate.
> >
> > Not sure if it would help as that has
> >
> > /* ??? Similarly, e.g. with (subreg:DF (reg:TI)). Though
> > store_bit_field
> > is the culprit here, and not the backends. */
> > else if (osize >= UNITS_PER_WORD && isize >= osize)
> > ;
> >
> > and thus we'd return true anyway for (subreg:XF (reg:TI) 0)
>
> If XFmode subreg of TImode reg passes validation, where does it ICE then?
It ICEs in
/space/rguenther/src/svn/trunk/gcc/testsuite/gcc.target/i386/pr37870.c:19:1:
internal compiler error: in subreg_get_info, at rtlanal.c:3695
0xddee5a subreg_get_info(unsigned int, machine_mode, unsigned int,
machine_mode, subreg_info*)
/space/rguenther/src/svn/trunk/gcc/rtlanal.c:3695
0xddf12b simplify_subreg_regno(unsigned int, machine_mode, unsigned int,
machine_mode)
/space/rguenther/src/svn/trunk/gcc/rtlanal.c:3808
0xd8bc7a simplifiable_subregs(subreg_shape const&)
/space/rguenther/src/svn/trunk/gcc/reginfo.c:1234
0xd8be1d record_subregs_of_mode
/space/rguenther/src/svn/trunk/gcc/reginfo.c:1294
0xd8c246 init_subregs_of_mode()
/space/rguenther/src/svn/trunk/gcc/reginfo.c:1348
0xc1a55c init_costs
/space/rguenther/src/svn/trunk/gcc/ira-costs.c:2187
which is
/* This should always pass, otherwise we don't know how to verify
the constraint. These conditions may be relaxed but
subreg_regno_offset would need to be redesigned. */
gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);
Richard.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1
2016-04-04 13:27 ` Richard Biener
@ 2016-04-04 13:33 ` Jakub Jelinek
2016-04-18 9:26 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2016-04-04 13:33 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, iant
On Mon, Apr 04, 2016 at 03:27:23PM +0200, Richard Biener wrote:
> It ICEs in
>
> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.target/i386/pr37870.c:19:1:
> internal compiler error: in subreg_get_info, at rtlanal.c:3695
> 0xddee5a subreg_get_info(unsigned int, machine_mode, unsigned int,
> machine_mode, subreg_info*)
> /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3695
> 0xddf12b simplify_subreg_regno(unsigned int, machine_mode, unsigned int,
> machine_mode)
> /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3808
> 0xd8bc7a simplifiable_subregs(subreg_shape const&)
> /space/rguenther/src/svn/trunk/gcc/reginfo.c:1234
> 0xd8be1d record_subregs_of_mode
> /space/rguenther/src/svn/trunk/gcc/reginfo.c:1294
> 0xd8c246 init_subregs_of_mode()
> /space/rguenther/src/svn/trunk/gcc/reginfo.c:1348
> 0xc1a55c init_costs
> /space/rguenther/src/svn/trunk/gcc/ira-costs.c:2187
>
> which is
>
> /* This should always pass, otherwise we don't know how to verify
> the constraint. These conditions may be relaxed but
> subreg_regno_offset would need to be redesigned. */
> gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);
So perhaps either validate_subreg should check the same thing, or
the extraction could check this.
Anyway, I don't have anything against removing that hunk from
extract_bit_field_1 for GCC7.
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1
2016-04-04 13:33 ` Jakub Jelinek
@ 2016-04-18 9:26 ` Richard Biener
0 siblings, 0 replies; 7+ messages in thread
From: Richard Biener @ 2016-04-18 9:26 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: GCC Patches, Ian Lance Taylor
On Mon, Apr 4, 2016 at 3:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Apr 04, 2016 at 03:27:23PM +0200, Richard Biener wrote:
>> It ICEs in
>>
>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.target/i386/pr37870.c:19:1:
>> internal compiler error: in subreg_get_info, at rtlanal.c:3695
>> 0xddee5a subreg_get_info(unsigned int, machine_mode, unsigned int,
>> machine_mode, subreg_info*)
>> /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3695
>> 0xddf12b simplify_subreg_regno(unsigned int, machine_mode, unsigned int,
>> machine_mode)
>> /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3808
>> 0xd8bc7a simplifiable_subregs(subreg_shape const&)
>> /space/rguenther/src/svn/trunk/gcc/reginfo.c:1234
>> 0xd8be1d record_subregs_of_mode
>> /space/rguenther/src/svn/trunk/gcc/reginfo.c:1294
>> 0xd8c246 init_subregs_of_mode()
>> /space/rguenther/src/svn/trunk/gcc/reginfo.c:1348
>> 0xc1a55c init_costs
>> /space/rguenther/src/svn/trunk/gcc/ira-costs.c:2187
>>
>> which is
>>
>> /* This should always pass, otherwise we don't know how to verify
>> the constraint. These conditions may be relaxed but
>> subreg_regno_offset would need to be redesigned. */
>> gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);
>
> So perhaps either validate_subreg should check the same thing, or
> the extraction could check this.
> Anyway, I don't have anything against removing that hunk from
> extract_bit_field_1 for GCC7.
Now committed.
Richard.
> Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-18 9:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-04 12:57 [PATCH][GCC 7] Remove broken path in extract_bit_field_1 Richard Biener
2016-04-04 13:06 ` Jakub Jelinek
2016-04-04 13:10 ` Richard Biener
2016-04-04 13:24 ` Jakub Jelinek
2016-04-04 13:27 ` Richard Biener
2016-04-04 13:33 ` Jakub Jelinek
2016-04-18 9:26 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).