patch for elimination to SP when it is changed in RTL (PR57293)

On 12/1/2013, 7:57 AM, James Greenhalgh wrote:
> On Thu, Nov 28, 2013 at 10:11:26PM +0000, Vladimir Makarov wrote:
>> Committed as rev. 205498.
>>
>>    2013-11-28  Vladimir Makarov<vmakarov@redhat.com>
>>
>> 	PR target/57293
>> 	* ira.h (ira_setup_eliminable_regset): Remove parameter.
>> 	* ira.c (ira_setup_eliminable_regset): Ditto.  Add
>> 	SUPPORTS_STACK_ALIGNMENT for crtl->stack_realign_needed.
>> 	Don't call lra_init_elimination.
>> 	(ira): Call ira_setup_eliminable_regset without arguments.
>> 	* loop-invariant.c (calculate_loop_reg_pressure): Remove argument
>> 	from ira_setup_eliminable_regset call.
>> 	* gcse.c (calculate_bb_reg_pressure): Ditto.
>> 	* haifa-sched.c (sched_init): Ditto.
>> 	* lra.h (lra_init_elimination): Remove the prototype.
>> 	* lra-int.h (lra_insn_recog_data): New member sp_offset.  Move
>> 	used_insn_alternative upper.
>> 	(lra_eliminate_regs_1): Add one more parameter.
>> 	(lra-eliminate): Ditto.
>> 	* lra.c (lra_invalidate_insn_data): Set sp_offset.
>> 	(setup_sp_offset): New.
>> 	(lra_process_new_insns): Call setup_sp_offset.
>> 	(lra): Add argument to lra_eliminate calls.
>> 	* lra-constraints.c (get_equiv_substitution): Rename to get_equiv.
>> 	(get_equiv_with_elimination): New.
>> 	(process_addr_reg): Call get_equiv_with_elimination instead of
>> 	get_equiv_substitution.
>> 	(equiv_address_substitution): Ditto.
>> 	(loc_equivalence_change_p): Ditto.
>> 	(loc_equivalence_callback, lra_constraints): Ditto.
>> 	(curr_insn_transform): Ditto.  Print the sp offset
>> 	(process_alt_operands): Prevent stack pointer reloads.
>> 	(lra_constraints): Remove one argument from lra_eliminate call.
>> 	Move it up.  Mark used hard regs bfore it.  Use
>> 	get_equiv_with_elimination instead of get_equiv_substitution.
>> 	* lra-eliminations.c (lra_eliminate_regs_1): Add parameter and
>> 	assert for param values combination.  Use sp offset.  Add argument
>> 	to lra_eliminate_regs_1 calls.
>> 	(lra_eliminate_regs): Add argument to lra_eliminate_regs_1 call.
>> 	(curr_sp_change): New static var.
>> 	(mark_not_eliminable): Add parameter.  Update curr_sp_change.
>> 	Don't prevent elimination to sp if we can calculate its change.
>> 	Pass the argument to mark_not_eliminable calls.
>> 	(eliminate_regs_in_insn): Add a parameter.  Use sp offset.  Add
>> 	argument to lra_eliminate_regs_1 call.
>> 	(update_reg_eliminate): Move calculation of hard regs for spill
>> 	lower.  Switch off lra_in_progress temporarily to generate regs
>> 	involved into elimination.
>> 	(lra_init_elimination): Rename to init_elimination.  Make it
>> 	static.  Set up insn sp offset, check the offsets at the end of
>> 	BBs.
>> 	(process_insn_for_elimination): Add parameter.  Pass its value to
>> 	eliminate_regs_in_insn.
>> 	(lra_eliminate): : Add parameter.  Pass its value to
>> 	process_insn_for_elimination.  Add assert for param values
>> 	combination.  Call init_elimination.  Don't update offsets in
>> 	equivalence substitutions.
>> 	* lra-spills.c (assign_mem_slot): Don't call lra_eliminate_regs_1
>> 	for created stack slot.
>> 	(remove_pseudos): Call lra_eliminate_regs_1 before changing memory
>> 	onto stack slot.
>>
>> 2013-11-28  Vladimir Makarov<vmakarov@redhat.com>
>>
>> 	PR target/57293
>> 	* gcc.target/i386/pr57293.c: New.
>
> Hi Vlad,
>
> This patch seems to cause some problems for AArch64. I see an assert
> triggering when building libgloss:
>
> /work/gcc-clean/build-aarch64-none-elf/obj/gcc1/gcc/xgcc -B/work/gcc-clean/build-aarch64-none-elf/obj/gcc1/gcc/ -B/work/gcc-clean/build-aarch64-none-elf/obj/binutils/aarch64-none-elf/newlib/ -isystem /work/gcc-clean/build-aarch64-none-elf/obj/binutils/aarch64-none-elf/newlib/targ-include -isystem /work/gcc-clean/src/binutils/newlib/libc/include -B/work/gcc-clean/build-aarch64-none-elf/obj/binutils/aarch64-none-elf/libgloss/aarch64 -L/work/gcc-clean/build-aarch64-none-elf/obj/binutils/aarch64-none-elf/libgloss/libnosys -L/work/gcc-clean/src/binutils/libgloss/aarch64 -L/work/gcc-clean/build-aarch64-none-elf/obj/binutils/./ld    -O2 -g -O2 -g -I. -I/work/gcc-clean/src/binutils/libgloss/aarch64/.. -DARM_RDI_MONITOR -o rdimon-_exit.o -c /work/gcc-clean/src/binutils/libgloss/aarch64/_exit.c
> /work/gcc-clean/src/binutils/libgloss/aarch64/_exit.c: In function '_exit':
> /work/gcc-clean/src/binutils/libgloss/aarch64/_exit.c:41:1: internal compiler error: in update_reg_eliminate, at lra-eliminations.c:1157
>   }
>   ^
>

   First of all, it is a bad situation for code performance when IRA
decides that it can use frame pointer for allocation, and after that
LRA/reload decides that frame pointer can not be used and spills all
pseudos assigned to FP.  The generated code will be much worse than
one generated if we decided not to use FP from the IRA start.

   Therefore I decided to put an assert for checking the situation.  It
was triggered by your case.  I think the problem in the following
code.

aarch64_can_eliminate (const int from, const int to)
{
   if (frame_pointer_needed)
      ...
   else
     {
       ...
       if (from == FRAME_POINTER_REGNUM && to == STACK_POINTER_REGNUM
	  && df_regs_ever_live_p (LR_REGNUM)
	  && faked_omit_frame_pointer)
	return false;
     }
   return true;
}

and

static bool
aarch64_frame_pointer_required (void)
{
   ...
   if (flag_omit_frame_pointer && !faked_omit_frame_pointer)
     return false;
   else if (flag_omit_leaf_frame_pointer)
     return !crtl->is_leaf;
   return true;
}

   IRA calls hook frame_pointer_required and it returns false.  After
that LRA calls can_eliminate hook and it returns false which means
that fp can not be used for allocation and we should spill all pseudos
assigned to it.

   So the problem can be solved by modifying frame_pointer_required
hook.

   I also don't like

       if (from == FRAME_POINTER_REGNUM && to == STACK_POINTER_REGNUM
           && df_regs_ever_live_p (LR_REGNUM)
           && faked_omit_frame_pointer)
         return false;

   It means we cannot eliminate FP by SP but we still can eliminate AP
by SP, for example.  I think it is wrong.

So the following patch solves the problem and improves generated code.

If somebody with the rights approves, I can commit it tomorrow.

2013-12-02  Vladimir Makarov  <vmakarov@redhat.com>

	* config/aarch64/aarch64.c (aarch64_frame_pointer_required): Check
	LR_REGNUM.
	(aarch64_can_eliminate): Don't check elimination source when
	frame_pointer_requred is false.

patch for elimination to SP when it is changed in RTL (PR57293)

Commit Message

Comments

Patch