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

login
register
mail settings
Submitter Richard Sandiford
Date Aug. 6, 2013, 9:33 p.m.
Message ID <87pptq5wsc.fsf@talisman.default>
Download mbox | patch
Permalink /patch/265246/
State New
Headers show

Comments

Richard Sandiford - Aug. 6, 2013, 9:33 p.m.
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.
Richard Guenther - Aug. 7, 2013, 11:41 a.m.
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);
>+ }
>+}
Jürgen Urban - Aug. 7, 2013, 9:22 p.m.
> 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

Patch

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);
+ }
+}