[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #11 from Jakub Jelinek --- (In reply to Uroš Bizjak from comment #10) > (In reply to Jakub Jelinek from comment #9) > Years ago, I defined HAVE_PRE_DEC, and I was told that it is intended for > targets that can increment pointer on *any* memory access, not just on stack > pointer. So, x86 uses PRE_DEC and POST_INC only to abstract push and pop > insn. Various parts of the compiler (modulo auto-inc-dec.c) mostly check for > PRE_DEC and POST_INC to skip further optimizations, which is non-issue for > push and pop. > > But this generic part of the code assumes that any target can handle > PRE_MODIFY, and emits insn even for !HAVE_PRE_MODIFY_DISP. This is > conservatively incorrect, so conditionally disabling the part that blindly > emits PRE_MODIFY is IMO the way to go. But isn't that the same? Targets can only define HAVE_PRE_MODIFY_DISP, if they support PRE_MODIFY with arbitrary registers, but when they only support it with the stack pointer, they can't. > Maybe this issue should be discussed on the ML. Maybe. This matters only for targets that define PUSH_ROUNDING and define it to something other than unconditionally the first argument. I think that is cr16, h8300, i386, m32c, m68k, pdp11, sh, stormy16 None of these targets define HAVE_PRE_MODIFY_DISP, though h8300 clearly has patterns with pre_modify on stack_pointer_rtx (thus relies on the patch I've attached not to be applied), and i386.c, m32c.c, pdp11.c stormy16.c all have PRE_MODIFY tests at least in one place, e.g. pdp11.c has: /* accept -(SP) -- which uses PRE_MODIFY for byte mode */ comment (and code inspection agrees with that), so at least pdp11 and h8300 would be broken by that change, maybe m32c and stormy16 too. > BTW: > > - dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, > - gen_int_mode (offset, Pmode)); > + if (offset) > + dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, > + gen_int_mode (offset, Pmode)); > + else > + dest_addr = stack_pointer_rtx; > > plus_constant? Sure, iff we want to do that change.
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #10 from Uroš Bizjak --- (In reply to Jakub Jelinek from comment #9) > Created attachment 40868 [details] > gcc7-pr79514.patch > > You mean like this? That certainly works for x86, but I'm afraid it is > going to break or penalize various other arches. > The thing is, HAVE_PRE_MODIFY_DISP is set to 1 only on 5 targets, but quick > grep reveals that there are many more targets that somehow handle pre_modify > or have insn patterns using that. This is not unlike HAVE_PRE_DEC and > similar, x86 also doesn't claim it has HAVE_PRE_DEC, yet it uses it for > pushes. > So I think pushxf1 expander in i386.md is the way to go. Years ago, I defined HAVE_PRE_DEC, and I was told that it is intended for targets that can increment pointer on *any* memory access, not just on stack pointer. So, x86 uses PRE_DEC and POST_INC only to abstract push and pop insn. Various parts of the compiler (modulo auto-inc-dec.c) mostly check for PRE_DEC and POST_INC to skip further optimizations, which is non-issue for push and pop. But this generic part of the code assumes that any target can handle PRE_MODIFY, and emits insn even for !HAVE_PRE_MODIFY_DISP. This is conservatively incorrect, so conditionally disabling the part that blindly emits PRE_MODIFY is IMO the way to go. Maybe this issue should be discussed on the ML. BTW: - dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, - gen_int_mode (offset, Pmode)); + if (offset) + dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, + gen_int_mode (offset, Pmode)); + else + dest_addr = stack_pointer_rtx; plus_constant?
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #9 from Jakub Jelinek --- Created attachment 40868 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40868=edit gcc7-pr79514.patch You mean like this? That certainly works for x86, but I'm afraid it is going to break or penalize various other arches. The thing is, HAVE_PRE_MODIFY_DISP is set to 1 only on 5 targets, but quick grep reveals that there are many more targets that somehow handle pre_modify or have insn patterns using that. This is not unlike HAVE_PRE_DEC and similar, x86 also doesn't claim it has HAVE_PRE_DEC, yet it uses it for pushes. So I think pushxf1 expander in i386.md is the way to go.
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #8 from Uroš Bizjak --- (In reply to Jakub Jelinek from comment #7) > (In reply to Uroš Bizjak from comment #6) > > (In reply to Uroš Bizjak from comment #5) > > > Maybe we should use our own pushxf expander? > > > > No. Middle end should be fixed. x86 does not define any of HAVE_PRE_MODIFY > > macros. > > So what it can emit? PRE_DEC is not really right, because GET_MODE_SIZE > (XFmode) is in that case 12, but PUSH_ROUNDING increases that to 16. But > the push actually needs to adjust sp by 16, not 12. > With -m128bit-long-double, rounded_size is equal to GET_MODE_SIZE and we get > to: > 4124if (GET_MODE_SIZE (mode) == rounded_size) > 4125 dest_addr = gen_rtx_fmt_e (STACK_PUSH_CODE, Pmode, > stack_pointer_rtx); > I think i386.md should have a pushxf1 expander enabled at least for the case > when GET_MODE_SIZE (XFmode) != PUSH_ROUNDING (GET_MODE_SIZE (XFmode)) and > just not use PRE_DEC in the pattern at all, just sp subtraction plus store. Just below the code you quoted is some fixup code, depending on FUNCTION_ARG_PADDING () == downward. I wonder, why similar fixup can't be also applied for upward padding (the referred code)? The proposed XFmode expander would do just that - adjust the stack pointer reg and move the value to adjusted position, as is already the case with a similar x86 splitter (... which, according to its %%% comment, is a workaround by itself and should be killed). So, while introducing expander would solve this particular problem for x86, it would leave other targets unfixed. IMO, this problem should be fixed in the middle-end, not worked around with long-lasting target workarounds.
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #7 from Jakub Jelinek --- (In reply to Uroš Bizjak from comment #6) > (In reply to Uroš Bizjak from comment #5) > > Maybe we should use our own pushxf expander? > > No. Middle end should be fixed. x86 does not define any of HAVE_PRE_MODIFY > macros. So what it can emit? PRE_DEC is not really right, because GET_MODE_SIZE (XFmode) is in that case 12, but PUSH_ROUNDING increases that to 16. But the push actually needs to adjust sp by 16, not 12. With -m128bit-long-double, rounded_size is equal to GET_MODE_SIZE and we get to: 4124 if (GET_MODE_SIZE (mode) == rounded_size) 4125dest_addr = gen_rtx_fmt_e (STACK_PUSH_CODE, Pmode, stack_pointer_rtx); I think i386.md should have a pushxf1 expander enabled at least for the case when GET_MODE_SIZE (XFmode) != PUSH_ROUNDING (GET_MODE_SIZE (XFmode)) and just not use PRE_DEC in the pattern at all, just sp subtraction plus store.
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #6 from Uroš Bizjak --- (In reply to Uroš Bizjak from comment #5) > Maybe we should use our own pushxf expander? No. Middle end should be fixed. x86 does not define any of HAVE_PRE_MODIFY macros.
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514 --- Comment #5 from Uroš Bizjak --- We go through: if (STACK_GROWS_DOWNWARD) /* ??? This seems wrong if STACK_PUSH_CODE == POST_DEC. */ dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, gen_int_mode (-(HOST_WIDE_INT) rounded_size, Pmode)); else /* ??? This seems wrong if STACK_PUSH_CODE == POST_INC. */ dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, gen_int_mode (rounded_size, Pmode)); dest_addr = gen_rtx_PRE_MODIFY (Pmode, stack_pointer_rtx, dest_addr); Maybe we should use our own pushxf expander?
[Bug middle-end/79514] [5/6/7 Regression] ICE in curr_insn_transform, at lra-constraints.c:3773
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79514
Uroš Bizjak changed:
What|Removed |Added
Component|target |middle-end
--- Comment #4 from Uroš Bizjak ---
(In reply to Uroš Bizjak from comment #3)
> (In reply to Gerhard Steinmetz from comment #0)
> > (insn 10 9 11 2 (set (mem:XF (pre_modify:DI (reg/f:DI 7 sp)
> > (plus:DI (reg/f:DI 7 sp)
> > (const_int -16 [0xfff0]))) [2 S12 A32])
> > (reg:XF 87 [ _1 ])) "pr46251.c":3 118 {*pushxf}
> > (expr_list:REG_DEAD (reg:XF 87 [ _1 ])
> > (expr_list:REG_ARGS_SIZE (const_int 16 [0x10])
> > (nil
>
> This doesn't look like a valid RTL to me.
A bit of debugging shows:
Breakpoint 1, gen_movxf (operand0=0x2e8f3318, operand1=0x2e8daf90) at
../../git/gcc/gcc/config/i386/i386.md:3185
3185 return standard_sse_constant_opcode (insn, operands[1]);
(gdb) p debug_rtx (operand1)
(reg:XF 87 [ _1 ])
$3 = void
(gdb) p debug_rtx (operand0)
(mem:XF (pre_modify:DI (reg/f:DI 7 sp)
(plus:DI (reg/f:DI 7 sp)
(const_int -16 [0xfff0]))) [2 S12 A32])
$4 = void
(gdb) bt
#0 gen_movxf (operand0=0x2e8f3318, operand1=0x2e8daf90) at
../../git/gcc/gcc/config/i386/i386.md:3185
#1 0x007e23ef in operator() (a1=0x2e8daf90, a0=0x2e8f3318,
this=) at ../../git/gcc/gcc/recog.h:301
#2 emit_move_insn_1 (x=0x2e8f3318, y=0x2e8daf90) at
../../git/gcc/gcc/expr.c:3643
#3 0x007e2745 in emit_move_insn (x=x@entry=0x2e8f3318,
y=y@entry=0x2e8daf90) at ../../git/gcc/gcc/expr.c:3738
#4 0x007e4433 in emit_single_push_insn_1 (type=0x2e7c8498,
x=0x2e8daf90, mode=) at ../../git/gcc/gcc/expr.c:4185
#5 emit_single_push_insn (mode=, x=0x2e8daf90,
type=0x2e7c8498) at ../../git/gcc/gcc/expr.c:4197
#6 0x007e817b in emit_push_insn (x=0x2e8daf90, mode=XFmode,
type=0x2e7c8498, size=0x0, align=64, partial=0, reg=0x0, extra=0,
args_addr=0x0, args_so_far=0x2e7b0480, reg_parm_stack_space=0,
alignment_pad=0x2e7b0480, sibcall_p=true)
at ../../git/gcc/gcc/expr.c:4536
...
(gdb) f 4
#4 0x007e4433 in emit_single_push_insn_1 (type=0x2e7c8498,
x=0x2e8daf90, mode=) at ../../git/gcc/gcc/expr.c:4185
4185 emit_move_insn (dest, x);
(gdb) p debug_rtx (dest)
(mem:XF (pre_modify:DI (reg/f:DI 7 sp)
(plus:DI (reg/f:DI 7 sp)
(const_int -16 [0xfff0]))) [2 S12 A32])
$12 = void
(gdb) list
4180 outgoing arguments and we cannot allow reordering of reads
4181 from function arguments with stores to outgoing arguments
4182 of sibling calls. */
4183set_mem_alias_set (dest, 0);
4184}
4185 emit_move_insn (dest, x);
4186}
4187
4188/* Emit and annotate a single push insn. */
4189
So, it is middle end that generates invalid RTX.
