RFC/A: PR 58079: CONST_INT mode assert in combine.c:do_SUBST

RFC/A: PR 58079: CONST_INT mode assert in combine.c:do_SUBST

[Richard Sandiford]

PR 58079 is about the do_SUBST assert:

      /* Sanity check that we're replacing oldval with a CONST_INT
	 that is a valid sign-extension for the original mode.  */
      gcc_assert (INTVAL (newval)
		  == trunc_int_for_mode (INTVAL (newval), GET_MODE (oldval)));

triggering while trying to optimise the temporary result:

  (eq (const_int -73 [0xffffffffffffffb7])
      (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ])

combine_simplify_rtx calls simplify_comparison, which first canonicalises
the order so that the constant is second and then promotes the comparison
to SImode (the first supported comparison mode on MIPS).  So we end up with:

  (eq (zero_extend:SI (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ]))
      (const_int 183 [0xb7]))

So far so good.  But combine_simplify_rtx then tries to install the
new operands in-place, with the second part of:

	  /* If the code changed, return a whole new comparison.  */
	  if (new_code != code)
	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);

	  /* Otherwise, keep this operation, but maybe change its operands.
	     This also converts (ne (compare FOO BAR) 0) to (ne FOO BAR).  */
	  SUBST (XEXP (x, 0), op0);
	  SUBST (XEXP (x, 1), op1);

And this triggers the assert because we're replacing a QImode register
with the non-QImode constant 183.

One fix would be to extend the new_code != code condition, as below.
This should avoid the problem cases without generating too much
garbage rtl.  But if the condition's getting this complicated,
perhaps it'd be better just to avoid SUBST here?  I.e.

	  if (new_code != code || op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);

Just asking though. :-)

Tested on mipsisa64-elf.  OK to install?

Thanks,
Richard


gcc/
	PR rtl-optimization/58079
	* combine.c (combine_simplify_rtx): Avoid using SUBST if
	simplify_comparison has widened a comparison with an integer.

gcc/testsuite/
	* gcc.dg/torture/pr58079.c: New test.

Index: gcc/combine.c
===================================================================
--- gcc/combine.c	2013-08-06 22:03:32.644642305 +0100
+++ gcc/combine.c	2013-08-06 22:08:51.944653901 +0100
@@ -5803,8 +5803,15 @@ combine_simplify_rtx (rtx x, enum machin
 		return x;
 	    }
 
-	  /* If the code changed, return a whole new comparison.  */
-	  if (new_code != code)
+	  /* If the code changed, return a whole new comparison.
+	     We also need to avoid using SUBST in cases where
+	     simplify_comparison has widened a comparison with a CONST_INT,
+	     since in that case the wider CONST_INT may fail the sanity
+	     checks in do_SUBST.  */
+	  if (new_code != code
+	      || (CONST_INT_P (op1)
+		  && GET_MODE (op0) != GET_MODE (XEXP (x, 0))
+		  && GET_MODE (op0) != GET_MODE (XEXP (x, 1))))
 	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
 
 	  /* Otherwise, keep this operation, but maybe change its operands.
Index: gcc/testsuite/gcc.dg/torture/pr58079.c
===================================================================
--- /dev/null	2013-07-23 18:41:43.074412242 +0100
+++ gcc/testsuite/gcc.dg/torture/pr58079.c	2013-08-06 22:05:06.249523398 +0100
@@ -0,0 +1,107 @@
+/* { dg-options "-mlong-calls" { target mips*-*-* } } */
+
+typedef unsigned char u8;
+typedef unsigned short u16;
+typedef unsigned int __kernel_size_t;
+typedef __kernel_size_t size_t;
+struct list_head {
+ struct list_head *next;
+};
+
+struct dmx_ts_feed {
+ int is_filtering;
+};
+struct dmx_section_feed {
+ u16 secbufp;
+ u16 seclen;
+ u16 tsfeedp;
+};
+
+typedef int (*dmx_ts_cb) (
+	const u8 * buffer1,
+      size_t buffer1_length,
+      const u8 * buffer2,
+      size_t buffer2_length
+);
+
+struct dvb_demux_feed {
+ union {
+  struct dmx_ts_feed ts;
+  struct dmx_section_feed sec;
+ } feed;
+ union {
+  dmx_ts_cb ts;
+ } cb;
+ int type;
+ u16 pid;
+ int ts_type;
+ struct list_head list_head;
+};
+
+struct dvb_demux {
+ int (*stop_feed)(struct dvb_demux_feed *feed);
+ struct list_head feed_list;
+};
+
+
+static
+inline
+__attribute__((always_inline))
+u8
+payload(const u8 *tsp)
+{
+ if (tsp[3] & 0x20) {
+   return 184 - 1 - tsp[4];
+ }
+ return 184;
+}
+
+static
+inline
+__attribute__((always_inline))
+int
+dvb_dmx_swfilter_payload(struct dvb_demux_feed *feed, const u8 *buf)
+{
+ int count = payload(buf);
+ int p;
+ if (count == 0)
+  return -1;
+ return feed->cb.ts(&buf[p], count, ((void *)0), 0);
+}
+
+static
+inline
+__attribute__((always_inline))
+void
+dvb_dmx_swfilter_packet_type(struct dvb_demux_feed *feed, const u8 *buf)
+{
+ switch (feed->type) {
+ case 0:
+  if (feed->ts_type & 1) {
+    dvb_dmx_swfilter_payload(feed, buf);
+  }
+  if (dvb_dmx_swfilter_section_packet(feed, buf) < 0)
+   feed->feed.sec.seclen = feed->feed.sec.secbufp = 0;
+ }
+}
+
+static
+void
+dvb_dmx_swfilter_packet(struct dvb_demux *demux, const u8 *buf)
+{
+ struct dvb_demux_feed *feed;
+ int dvr_done = 0;
+
+ for (feed = ({ const typeof( ((typeof(*feed) *)0)->list_head ) *__mptr = ((&demux->feed_list)->next); (typeof(*feed) *)( (char *)__mptr - __builtin_offsetof(typeof(*feed),list_head) );}); __builtin_prefetch(feed->list_head.next), &feed->list_head != (&demux->feed_list); feed = ({ const typeof( ((typeof(*feed) *)0)->list_head ) *__mptr = (feed->list_head.next); (typeof(*feed) *)( (char *)__mptr - __builtin_offsetof(typeof(*feed),list_head) );})) {
+  if (((((feed)->type == 0) && ((feed)->feed.ts.is_filtering) && (((feed)->ts_type & (1 | 8)) == 1))) && (dvr_done++))
+   dvb_dmx_swfilter_packet_type(feed, buf);
+  else if (feed->pid == 0x2000)
+   feed->cb.ts(buf, 188, ((void *)0), 0);
+ }
+}
+void dvb_dmx_swfilter_packets(struct dvb_demux *demux, const u8 *buf, size_t count)
+{
+ while (count--) {
+   dvb_dmx_swfilter_packet(demux, buf);
+ }
+}

Re: RFC/A: PR 58079: CONST_INT mode assert in combine.c:do_SUBST

[Richard Biener]

Richard Sandiford <rdsandiford@googlemail.com> wrote:
>PR 58079 is about the do_SUBST assert:
>
>      /* Sanity check that we're replacing oldval with a CONST_INT
>	 that is a valid sign-extension for the original mode.  */
>      gcc_assert (INTVAL (newval)
>		  == trunc_int_for_mode (INTVAL (newval), GET_MODE (oldval)));
>
>triggering while trying to optimise the temporary result:
>
>  (eq (const_int -73 [0xffffffffffffffb7])
>      (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ])
>
>combine_simplify_rtx calls simplify_comparison, which first
>canonicalises
>the order so that the constant is second and then promotes the
>comparison
>to SImode (the first supported comparison mode on MIPS).  So we end up
>with:
>
>  (eq (zero_extend:SI (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ]))
>      (const_int 183 [0xb7]))
>
>So far so good.  But combine_simplify_rtx then tries to install the
>new operands in-place, with the second part of:
>
>	  /* If the code changed, return a whole new comparison.  */
>	  if (new_code != code)
>	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
>
>	  /* Otherwise, keep this operation, but maybe change its operands.
>	     This also converts (ne (compare FOO BAR) 0) to (ne FOO BAR).  */
>	  SUBST (XEXP (x, 0), op0);
>	  SUBST (XEXP (x, 1), op1);
>
>And this triggers the assert because we're replacing a QImode register
>with the non-QImode constant 183.
>
>One fix would be to extend the new_code != code condition, as below.
>This should avoid the problem cases without generating too much
>garbage rtl.  But if the condition's getting this complicated,
>perhaps it'd be better just to avoid SUBST here?  I.e.
>
>	  if (new_code != code || op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
>	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
>
>Just asking though. :-)
>
>Tested on mipsisa64-elf.  OK to install?

Looks reasonable to me. Thus ok if nobody objects within 24h.

Thanks,
Richard.

>Thanks,
>Richard
>
>
>gcc/
>	PR rtl-optimization/58079
>	* combine.c (combine_simplify_rtx): Avoid using SUBST if
>	simplify_comparison has widened a comparison with an integer.
>
>gcc/testsuite/
>	* gcc.dg/torture/pr58079.c: New test.
>
>Index: gcc/combine.c
>===================================================================
>--- gcc/combine.c	2013-08-06 22:03:32.644642305 +0100
>+++ gcc/combine.c	2013-08-06 22:08:51.944653901 +0100
>@@ -5803,8 +5803,15 @@ combine_simplify_rtx (rtx x, enum machin
> 		return x;
> 	    }
> 
>-	  /* If the code changed, return a whole new comparison.  */
>-	  if (new_code != code)
>+	  /* If the code changed, return a whole new comparison.
>+	     We also need to avoid using SUBST in cases where
>+	     simplify_comparison has widened a comparison with a CONST_INT,
>+	     since in that case the wider CONST_INT may fail the sanity
>+	     checks in do_SUBST.  */
>+	  if (new_code != code
>+	      || (CONST_INT_P (op1)
>+		  && GET_MODE (op0) != GET_MODE (XEXP (x, 0))
>+		  && GET_MODE (op0) != GET_MODE (XEXP (x, 1))))
> 	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
> 
> 	  /* Otherwise, keep this operation, but maybe change its operands.
>Index: gcc/testsuite/gcc.dg/torture/pr58079.c
>===================================================================
>--- /dev/null	2013-07-23 18:41:43.074412242 +0100
>+++ gcc/testsuite/gcc.dg/torture/pr58079.c	2013-08-06
>22:05:06.249523398 +0100
>@@ -0,0 +1,107 @@
>+/* { dg-options "-mlong-calls" { target mips*-*-* } } */
>+
>+typedef unsigned char u8;
>+typedef unsigned short u16;
>+typedef unsigned int __kernel_size_t;
>+typedef __kernel_size_t size_t;
>+struct list_head {
>+ struct list_head *next;
>+};
>+
>+struct dmx_ts_feed {
>+ int is_filtering;
>+};
>+struct dmx_section_feed {
>+ u16 secbufp;
>+ u16 seclen;
>+ u16 tsfeedp;
>+};
>+
>+typedef int (*dmx_ts_cb) (
>+	const u8 * buffer1,
>+      size_t buffer1_length,
>+      const u8 * buffer2,
>+      size_t buffer2_length
>+);
>+
>+struct dvb_demux_feed {
>+ union {
>+  struct dmx_ts_feed ts;
>+  struct dmx_section_feed sec;
>+ } feed;
>+ union {
>+  dmx_ts_cb ts;
>+ } cb;
>+ int type;
>+ u16 pid;
>+ int ts_type;
>+ struct list_head list_head;
>+};
>+
>+struct dvb_demux {
>+ int (*stop_feed)(struct dvb_demux_feed *feed);
>+ struct list_head feed_list;
>+};
>+
>+
>+static
>+inline
>+__attribute__((always_inline))
>+u8
>+payload(const u8 *tsp)
>+{
>+ if (tsp[3] & 0x20) {
>+   return 184 - 1 - tsp[4];
>+ }
>+ return 184;
>+}
>+
>+static
>+inline
>+__attribute__((always_inline))
>+int
>+dvb_dmx_swfilter_payload(struct dvb_demux_feed *feed, const u8 *buf)
>+{
>+ int count = payload(buf);
>+ int p;
>+ if (count == 0)
>+  return -1;
>+ return feed->cb.ts(&buf[p], count, ((void *)0), 0);
>+}
>+
>+static
>+inline
>+__attribute__((always_inline))
>+void
>+dvb_dmx_swfilter_packet_type(struct dvb_demux_feed *feed, const u8
>*buf)
>+{
>+ switch (feed->type) {
>+ case 0:
>+  if (feed->ts_type & 1) {
>+    dvb_dmx_swfilter_payload(feed, buf);
>+  }
>+  if (dvb_dmx_swfilter_section_packet(feed, buf) < 0)
>+   feed->feed.sec.seclen = feed->feed.sec.secbufp = 0;
>+ }
>+}
>+
>+static
>+void
>+dvb_dmx_swfilter_packet(struct dvb_demux *demux, const u8 *buf)
>+{
>+ struct dvb_demux_feed *feed;
>+ int dvr_done = 0;
>+
>+ for (feed = ({ const typeof( ((typeof(*feed) *)0)->list_head )
>*__mptr = ((&demux->feed_list)->next); (typeof(*feed) *)( (char
>*)__mptr - __builtin_offsetof(typeof(*feed),list_head) );});
>__builtin_prefetch(feed->list_head.next), &feed->list_head !=
>(&demux->feed_list); feed = ({ const typeof( ((typeof(*feed)
>*)0)->list_head ) *__mptr = (feed->list_head.next); (typeof(*feed) *)(
>(char *)__mptr - __builtin_offsetof(typeof(*feed),list_head) );})) {
>+  if (((((feed)->type == 0) && ((feed)->feed.ts.is_filtering) &&
>(((feed)->ts_type & (1 | 8)) == 1))) && (dvr_done++))
>+   dvb_dmx_swfilter_packet_type(feed, buf);
>+  else if (feed->pid == 0x2000)
>+   feed->cb.ts(buf, 188, ((void *)0), 0);
>+ }
>+}
>+void dvb_dmx_swfilter_packets(struct dvb_demux *demux, const u8 *buf,
>size_t count)
>+{
>+ while (count--) {
>+   dvb_dmx_swfilter_packet(demux, buf);
>+ }
>+}

Aw: RFC/A: PR 58079: CONST_INT mode assert in combine.c:do_SUBST

["Jürgen Urban"]

> PR 58079 is about the do_SUBST assert:
> 
>       /* Sanity check that we're replacing oldval with a CONST_INT
> 	 that is a valid sign-extension for the original mode.  */
>       gcc_assert (INTVAL (newval)
> 		  == trunc_int_for_mode (INTVAL (newval), GET_MODE (oldval)));
> 
> triggering while trying to optimise the temporary result:
> 
>   (eq (const_int -73 [0xffffffffffffffb7])
>       (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ])
> 
> combine_simplify_rtx calls simplify_comparison, which first canonicalises
> the order so that the constant is second and then promotes the comparison
> to SImode (the first supported comparison mode on MIPS).  So we end up with:
> 
>   (eq (zero_extend:SI (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ]))
>       (const_int 183 [0xb7]))
> 
> So far so good.  But combine_simplify_rtx then tries to install the
> new operands in-place, with the second part of:
> 
> 	  /* If the code changed, return a whole new comparison.  */
> 	  if (new_code != code)
> 	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
> 
> 	  /* Otherwise, keep this operation, but maybe change its operands.
> 	     This also converts (ne (compare FOO BAR) 0) to (ne FOO BAR).  */
> 	  SUBST (XEXP (x, 0), op0);
> 	  SUBST (XEXP (x, 1), op1);
> 
> And this triggers the assert because we're replacing a QImode register
> with the non-QImode constant 183.
> 
> One fix would be to extend the new_code != code condition, as below.
> This should avoid the problem cases without generating too much
> garbage rtl.  But if the condition's getting this complicated,
> perhaps it'd be better just to avoid SUBST here?  I.e.
> 
> 	  if (new_code != code || op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
> 	    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
> 
> Just asking though. :-)
> 
> Tested on mipsisa64-elf.  OK to install?

Yes, this fixes the error.

Thanks
Jürgen

Re: RFC/A: PR 58079: CONST_INT mode assert in combine.c:do_SUBST

[Uros Bizjak]

On Mon, Oct 28, 2013 at 5:28 PM, Uros Bizjak <ubizjak@gmail.com> wrote:

>> PR 58079 is about the do_SUBST assert:
>>
>>       /* Sanity check that we're replacing oldval with a CONST_INT
>> that is a valid sign-extension for the original mode.  */
>>       gcc_assert (INTVAL (newval)
>>  == trunc_int_for_mode (INTVAL (newval), GET_MODE (oldval)));
>>
>> triggering while trying to optimise the temporary result:
>>
>>   (eq (const_int -73 [0xffffffffffffffb7])
>>       (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ])
>>
>> combine_simplify_rtx calls simplify_comparison, which first canonicalises
>> the order so that the constant is second and then promotes the comparison
>> to SImode (the first supported comparison mode on MIPS).  So we end up with:
>>
>>   (eq (zero_extend:SI (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ]))
>>       (const_int 183 [0xb7]))
>>
>> So far so good.  But combine_simplify_rtx then tries to install the
>> new operands in-place, with the second part of:
>>
>>  /* If the code changed, return a whole new comparison.  */
>>  if (new_code != code)
>>    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
>>
>>  /* Otherwise, keep this operation, but maybe change its operands.
>>     This also converts (ne (compare FOO BAR) 0) to (ne FOO BAR).  */
>>  SUBST (XEXP (x, 0), op0);
>>  SUBST (XEXP (x, 1), op1);
>>
>> And this triggers the assert because we're replacing a QImode register
>> with the non-QImode constant 183.
>>
>> One fix would be to extend the new_code != code condition, as below.
>> This should avoid the problem cases without generating too much
>> garbage rtl.  But if the condition's getting this complicated,
>> perhaps it'd be better just to avoid SUBST here?  I.e.
>>
>>  if (new_code != code || op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
>>    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
>>
>> Just asking though. :-)
>>
>> Tested on mipsisa64-elf.  OK to install?
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> PR rtl-optimization/58079
>> * combine.c (combine_simplify_rtx): Avoid using SUBST if
>> simplify_comparison has widened a comparison with an integer.
>>
>> gcc/testsuite/
>> * gcc.dg/torture/pr58079.c: New test.
>
> I would like to backport this patch to 4.8 branch. The patch fixes the
> failure on alpha-linux-gnu and also (reportedly) fixes the same
> failure on mips and pa targets.
>
> I have bootstrapped and regression test the patch on
> x86_64-pc-linux-gnu {,-m32} and started bootstrap and regtests on
> alphaev68-linux-gnu on 4.8 branch for all default languages + obj-c++
> and go. The latter will take some ~10 hours.
>
> OK to backort the patch to the 4.8 branch if both tests pass?

The regression test was OK in both cases, so I went ahead and
installed the patch. The fix is kind of obvious anyway.

Uros.

Re: RFC/A: PR 58079: CONST_INT mode assert in combine.c:do_SUBST

[Uros Bizjak]

Hello!

> PR 58079 is about the do_SUBST assert:
>
>       /* Sanity check that we're replacing oldval with a CONST_INT
> that is a valid sign-extension for the original mode.  */
>       gcc_assert (INTVAL (newval)
>  == trunc_int_for_mode (INTVAL (newval), GET_MODE (oldval)));
>
> triggering while trying to optimise the temporary result:
>
>   (eq (const_int -73 [0xffffffffffffffb7])
>       (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ])
>
> combine_simplify_rtx calls simplify_comparison, which first canonicalises
> the order so that the constant is second and then promotes the comparison
> to SImode (the first supported comparison mode on MIPS).  So we end up with:
>
>   (eq (zero_extend:SI (reg:QI 234 [ MEM[(const u8 *)buf_7(D) + 4B] ]))
>       (const_int 183 [0xb7]))
>
> So far so good.  But combine_simplify_rtx then tries to install the
> new operands in-place, with the second part of:
>
>  /* If the code changed, return a whole new comparison.  */
>  if (new_code != code)
>    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
>
>  /* Otherwise, keep this operation, but maybe change its operands.
>     This also converts (ne (compare FOO BAR) 0) to (ne FOO BAR).  */
>  SUBST (XEXP (x, 0), op0);
>  SUBST (XEXP (x, 1), op1);
>
> And this triggers the assert because we're replacing a QImode register
> with the non-QImode constant 183.
>
> One fix would be to extend the new_code != code condition, as below.
> This should avoid the problem cases without generating too much
> garbage rtl.  But if the condition's getting this complicated,
> perhaps it'd be better just to avoid SUBST here?  I.e.
>
>  if (new_code != code || op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
>    return gen_rtx_fmt_ee (new_code, mode, op0, op1);
>
> Just asking though. :-)
>
> Tested on mipsisa64-elf.  OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> PR rtl-optimization/58079
> * combine.c (combine_simplify_rtx): Avoid using SUBST if
> simplify_comparison has widened a comparison with an integer.
>
> gcc/testsuite/
> * gcc.dg/torture/pr58079.c: New test.

I would like to backport this patch to 4.8 branch. The patch fixes the
failure on alpha-linux-gnu and also (reportedly) fixes the same
failure on mips and pa targets.

I have bootstrapped and regression test the patch on
x86_64-pc-linux-gnu {,-m32} and started bootstrap and regtests on
alphaev68-linux-gnu on 4.8 branch for all default languages + obj-c++
and go. The latter will take some ~10 hours.

OK to backort the patch to the 4.8 branch if both tests pass?

Thanks,
Uros.