Hi!
On Thu, Feb 10, 2022 at 10:55:03AM +0100, Jakub Jelinek wrote:
> The following testcase ICEs, because combine substitutes
> (insn 10 9 11 2 (set (reg/v:SI 7 sp [ a ])
> (const_int 0 [0])) "pr104446.c":9:5 81 {*movsi_internal}
> (nil))
> (insn 13 11 14 2 (set (mem/f:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32])
> (reg:SI 85)) "pr104446.c":10:3 56 {*pushsi2}
> (expr_list:REG_DEAD (reg:SI 85)
> (expr_list:REG_ARGS_SIZE (const_int 16 [0x10])
> (nil))))
> forming
> (insn 13 11 14 2 (set (mem/f:SI (pre_dec:SI (const_int 0 [0])) [0 S4 A32])
> (reg:SI 85)) "pr104446.c":10:3 56 {*pushsi2}
> (expr_list:REG_DEAD (reg:SI 85)
> (expr_list:REG_ARGS_SIZE (const_int 16 [0x10])
> (nil))))
> which is invalid RTL (pre_dec's argument must be a REG).
> I know substitution creates various forms of invalid RTL and hopes that
> invalid RTL just won't recog.
This is not a "hope"; it is a requirement. If the backend accepts
invalid insns, this is a bug in the backend.
> But unfortunately in this case we ICE before we get to recog, as
> try_combine does:
> if (n_auto_inc)
> {
> int new_n_auto_inc = 0;
> for_each_inc_dec (newpat, count_auto_inc, &new_n_auto_inc);
>
> if (n_auto_inc != new_n_auto_inc)
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "Number of auto_inc expressions changed\n");
> undo_all ();
> return 0;
> }
> }
> and for_each_inc_dec under the hood will do e.g. for the PRE_DEC case:
> case PRE_DEC:
> case POST_DEC:
> {
> poly_int64 size = GET_MODE_SIZE (GET_MODE (mem));
> rtx r1 = XEXP (x, 0);
> rtx c = gen_int_mode (-size, GET_MODE (r1));
> return fn (mem, x, r1, r1, c, data);
> }
> and that code rightfully expects that the PRE_DEC operand has non-VOIDmode
> (as it needs to be a REG) - gen_int_mode for VOIDmode results in ICE.
> I think it is better not to emit the clearly invalid RTL during substitution
> like we do for other cases, than to adding workarounds for invalid IL
> created by combine to rtlanal.cc and perhaps elsewhere.
But we do have that in other cases, and not just for combine. IMO it
is a good idea to robustify for_each_inc_dec (simply have it skip if the
address is not MODE_INT or such). It also is a good idea to robustify
combine subst, just as you do. It is best to do both!
> As for the testcase, of course it is UB at runtime to modify sp that way,
> but if such code is never reached, we must compile it, not to ICE on it.
It is an error at compile time already. The stack pointer is a fixed
register. The generic parts of the compiler use it, it is not just a
backend thing.
There are many more ways to ICE the compiler with register vars, btw.
And yes, ICEs are bad of course :-) QoI thing...
> And I don't see why on other targets which use the autoinc rtxes much more
> it couldn't happen with other registers.
Yes. My point (in the PR) was that it is easy enough to make this valid
code instead!
> PR middle-end/104446
> * combine.cc (subst): Don't substitute CONST_INTs into RTX_AUTOINC
> operands.
>
> * gcc.target/i386/pr104446.c: New test.
> +/* PR middle-end/104446 */
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2 -mrtd" } */
> +
> +register volatile int a __asm__("%esp");
> +void foo (void *);
> +void bar (void *);
> +
> +void
> +baz (void)
> +{
> + foo (__builtin_return_address (0));
> + a = 0;
> + bar (__builtin_return_address (0));
> +}
So does it not fail if you make this valid code (by using another
register)? bp, si, or di maybe?
Okay with that fixed. If fixing it is too hard, okay like this (I don't
have to maintain other peoples' backends' testsuites after all...)
Thanks!
Segher
