Richard Sandiford <rdsandif...@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);
>+ }
>+}
