Re: [PATCH V3 05/11] bpf: new GCC port

[Richard Sandiford <richard dot sandiford at arm dot com>]

jose.marchesi@oracle.com (Jose E. Marchesi) writes:
>         > +(define_expand "zero_extendsidi2"
>         > +  [(set (match_operand:DI 0 "register_operand")
>         > +	(zero_extend:DI (match_operand:SI 1 "reg_or_indirect_memory_operand")))]
>         > +  ""
>         > +{
>         > +  if (register_operand (operands[1], SImode))
>         > +    {
>         > +      operands[1] = gen_lowpart (DImode, operands[1]);
>         > +      emit_insn (gen_ashldi3 (operands[0], operands[1], GEN_INT (32)));
>         > +      emit_insn (gen_lshrdi3 (operands[0], operands[0], GEN_INT (32)));
>         > +      DONE;
>         > +    }
>         > +})
>         > +
>         > +(define_insn "*zero_extendsidi2"
>         > +  [(set (match_operand:DI 0 "register_operand" "=r,r")
>         > +	(zero_extend:DI (match_operand:SI 1 "reg_or_indirect_memory_operand" "0,m")))]
>         > +  ""
>         > +  "@
>         > +   lsh\t%0,32\n\trsh\t%0,32
>         > +   ldxw\t%0,%1"
>         > +  [(set_attr "type" "alu,ldx")
>         > +   (set_attr "length" "16,8")])
>         
>         Sorry, should have noticed last time, but: you shouldn't need to handle
>         register operands here given the expander above.  It's OK if you find it
>         improves code quality, but it'd be interesting to know why if so....
>     
>     If I remove the 0,=r alternative from the insn above, and also adjust
>     the predicate to indirect_memory_operand, then I get a segfault in one
>     test, in update_costs_from_allocno (ira-color.c:1382), because:
>     
>     (gdb) print mode
>     $1 = E_SImode
>     (gdb) print default_target_ira_int->x_ira_register_move_cost[mode]
>     $13 = (move_table *) 0x0
>     
>     What I think is going on is:
>     
>     1. The expand above is used, and
>     
>     2. there is no insn in the program matched by a pattern that involves a
>        SI operand, and therefore record_operand_costs is never called on
>        SImode operand, and therefore the lazily-initialized
>        x_ira_register_move_cost is never filled in for E_SImode, and then
>     
>     3. ira() -> ira_color() -> color () -> do_coloring () ->
>        ira_traverse_loop_tree () -> color_pass () -> color_allocnos () ->
>        update_costs_from_prefs () -> update_costs_from_allocno () *CRASH*
>     
>     Is this a bug, or am I expected to somehow trigger the initialization of
>     the SImode entry in the ira register move table in some other way?
>
> This is the backtrace btw:
>
> jemarch@termi:~/gnu/src/gcc-git/build-bpf/gcc$ PATH=.:$PATH ./xgcc -O2 -c /home/jemarch/gnu/src/gcc-git/gcc/testsuite/gcc.c-torture/compile/pr39928-2.c
> during RTL pass: ira
> /home/jemarch/gnu/src/gcc-git/gcc/testsuite/gcc.c-torture/compile/pr39928-2.c: In function ‘vq_nbest’:
> /home/jemarch/gnu/src/gcc-git/gcc/testsuite/gcc.c-torture/compile/pr39928-2.c:8:1: internal compiler error: Segmentation fault
>     8 | }
>       | ^
> 0xfa8cb7 crash_signal
> 	../../gcc/toplev.c:326
> 0x7f278d44e05f ???
> 	/build/glibc-yWQXbR/glibc-2.24/signal/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
> 0xd4ec8b update_costs_from_allocno
> 	../../gcc/ira-color.c:1382
> 0xd4ee09 update_costs_from_prefs
> 	../../gcc/ira-color.c:1422
> 0xd535d9 color_allocnos
> 	../../gcc/ira-color.c:3170
> 0xd53e21 color_pass
> 	../../gcc/ira-color.c:3309
> 0xd39b10 ira_traverse_loop_tree(bool, ira_loop_tree_node*, void (*)(ira_loop_tree_node*), void (*)(ira_loop_tree_node*))
> 	../../gcc/ira-build.c:1781
> 0xd54703 do_coloring
> 	../../gcc/ira-color.c:3460
> 0xd58203 color
> 	../../gcc/ira-color.c:4837
> 0xd58746 ira_color()
> 	../../gcc/ira-color.c:4968
> 0xd34990 ira
> 	../../gcc/ira.c:5365
> 0xd35118 execute
> 	../../gcc/ira.c:5663
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.

Yeah, this looks like a bug.  Further up we have:

      mode = ALLOCNO_MODE (allocno);
      ira_init_register_move_cost_if_necessary (mode);

which copes with the lazy initialisation.  But then we have:

	  mode = narrower_subreg_mode (mode, ALLOCNO_MODE (cp->second));
	  
	  cost = (cp->second == allocno
		  ? ira_register_move_cost[mode][rclass][aclass]
		  : ira_register_move_cost[mode][aclass][rclass]);

where we can change the mode without initialising the move cost tables.
Does it work if you change it to:

	  if (paradoxical_subreg_p (mode, ALLOCNO_MODE (cp->second)))
	    {
	      mode = ALLOCNO_MODE (cp->second);
	      ira_init_register_move_cost_if_necessary (mode);
	    }

?  The mode handling seems generally a bit weird though: we never
reset "mode" when processing the next copy, and "cp->second" might
be "allocno" itself, depending on which way round the move is.
But the above seems like an obvious change relative to the status quo.

Richard