diff mbox

Fix pr69012 ICE on building libgfortran for mips

Message ID HE1PR07MB0905471C32B184BD62E454CDE4FA0@HE1PR07MB0905.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Dec. 27, 2015, 7:09 p.m. UTC 
Hi,


the build of libgfortran and glibc for mips currently fails because we now evaluate
mips_compute_frame_info more often than before.  If any instruction uses
the predicate cprestore_save_slot_operand or cprestore_load_slot_operand
the function mips_cprestore_address_p calls mips_get_cprestore_base_and_offset
which needs valid data in frame->args_size and frame->hard_frame_pointer_offset,
but these are initialized after calling mips_global_pointer, which makes the function
get_attr_got raise an ICE.

This patch re-orders the frame structure initialization, to initialize the required data
before calling mips_global_pointer.

I built a cross-glibc and cross fortran and c++ compiler and verified that the ICE
is actually gone.

Is the patch ok for trunk?


Thanks
Bernd.
2015-12-27  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR target/69012
	* config/mips/mips.c (mips_compute_frame_info): Move call to
	mips_global_pointer after initialization of args_size and
	hard_frame_pointer_offset.

Comments

Richard Sandiford Dec. 30, 2015, 2:31 p.m. UTC | #1 
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> Hi,
>
>
> the build of libgfortran and glibc for mips currently fails because we
> now evaluate mips_compute_frame_info more often than before.  If any
> instruction uses the predicate cprestore_save_slot_operand or
> cprestore_load_slot_operand the function mips_cprestore_address_p
> calls mips_get_cprestore_base_and_offset which needs valid data in
> frame->args_size and frame->hard_frame_pointer_offset, but these are
> initialized after calling mips_global_pointer, which makes the
> function get_attr_got raise an ICE.
>
> This patch re-orders the frame structure initialization, to initialize
> the required data before calling mips_global_pointer.
>
> I built a cross-glibc and cross fortran and c++ compiler and verified
> that the ICE is actually gone.

I think the problem is deeper than that though.  The instructions
that are triggering the ICE are only generated by the prologue,
so this means that we're trying to lay out the frame again after
the prologue has been generated, whereas it really needs to be
fixed by then.  (And even if recalculating it is a no-op, the
operation is still too expensive to be repeated lightly.)

Query functions like rtx_addr_can_trap_p(_1) shouldn't really be
changing or recalculating the frame layout or other global state.
I think we need to find a different way of getting the information.

Maybe reload/LRA should use its own structures to calculate the
range of "safe" stack and hfp offsets, then store them in crtl
for rtx_addr_can_trap_p to use.  AIUI, before reload, the only
non-trapping uses of sp should be for outgoing arguments, so we
can use a test based on the cumulative outgoing arguments size.
I don't think the hfp should be used at all before reload, so we
could conservatively return -1 for that case.

Thanks,
Richard
diff mbox

Patch

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 231927)
+++ gcc/config/mips/mips.c	(working copy)
@@ -10347,8 +10347,6 @@  mips_compute_frame_info (void)
   memset (frame, 0, sizeof (*frame));
   size = get_frame_size ();
 
-  cfun->machine->global_pointer = mips_global_pointer ();
-
   /* The first two blocks contain the outgoing argument area and the $gp save
      slot.  This area isn't needed in leaf functions.  We can also skip it
      if we know that none of the called functions will use this space.
@@ -10377,6 +10375,17 @@  mips_compute_frame_info (void)
     }
   offset = frame->args_size + frame->cprestore_size;
 
+  /* MIPS16 code offsets the frame pointer by the size of the outgoing
+     arguments.  This tends to increase the chances of using unextended
+     instructions for local variables and incoming arguments.  */
+  if (TARGET_MIPS16)
+    frame->hard_frame_pointer_offset = frame->args_size;
+
+  /* The function mips_global_pointer walks all insns and some depend
+     on the predicate function mips_cprestore_address_p, which uses
+     frame->args_size and frame->hard_frame_pointer_offset.  */
+  cfun->machine->global_pointer = mips_global_pointer ();
+
   /* Move above the local variables.  */
   frame->var_size = MIPS_STACK_ALIGN (size);
   offset += frame->var_size;
@@ -10520,12 +10529,6 @@  mips_compute_frame_info (void)
     frame->acc_save_offset = frame->acc_sp_offset - offset;
   if (frame->num_cop0_regs > 0)
     frame->cop0_save_offset = frame->cop0_sp_offset - offset;
-
-  /* MIPS16 code offsets the frame pointer by the size of the outgoing
-     arguments.  This tends to increase the chances of using unextended
-     instructions for local variables and incoming arguments.  */
-  if (TARGET_MIPS16)
-    frame->hard_frame_pointer_offset = frame->args_size;
 }
 
 /* Return the style of GP load sequence that is being used for the