This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: x86 PIC regressions


On Wed, Sep 02, 1998 at 02:07:15PM -0500, Robert Lipe wrote:
> It now fails to bootstrap for me on either i686-pc-linux-gnu or
> i686-pc-sco3.2v5.0.5.   Both targets abort in the same place.  Reverting
> Richard's patch from this morning allows the bootstrap to finish.

What a pain in the ass.  Push the bubble down here, and it pops
up there.  With this one, I don't see any regressions; Uli is
running glibc tests for me now -- we'll see what happens there.

There are a couple of things:

  * While we generally get better code out of the compiler the earlier
    we validate memory addresses, force_const_mem is oft called in
    places that aren't expecting any instructions at all to be emitted.
    They somehow expect a mem to magically appear.  Which, particularly
    for ante-reload passes, seems wrong to me.

    I punt on this and just call validate_mem from mov[sdx]f, which 
    gets 99% of the cases.

  * The other 1% comes from reload taking a REG_EQUIV note and 
    reloading from constant memory.  But because of the way things
    work out, it means that we have to be prepared to accept

	(mem (symbol_ref/u "..."))

    even though a direct symbol_ref is not a valid address mode
    when PIC is enabled. 

    Taking into account that the only form I expect this to take is
    a load from the constant pool, I transform this into a legitimate
    pic address in the fp move expander.

  * A good bit of the reload nastiness was due to the incomplete job
    that was done supporting standard_80387_constant_p. 

    Previously, we exempted these from the early drop to memory in favour
    of letting them hang out, hoping that we'd issue an "fldz" or "fld1"
    to get them in the register.  Except that we forgot to tell reload
    about this, so it would almost always drop the constant to memory
    anyway.

    This is the point of the PREFERRED_RELOAD_CLASS change.

    Now, talking to Uli, it seems that it is not cut-and-dry whether
    it is even good to use this method.  Loading from memory with a
    hot cache line (as in a loop) takes one cycle, and pairs with
    everything.  The fldz insn, otoh, doesn't pair at all.  In either
    case, I think my patch is correct, and if we want to change the
    behaviour wrt fldz, we simply modify standard_80387_constant_p.

  * The REG_NOTES mucking is useless, as they won't get past
    gen_sequence+emit_insn.  It is obvious no one ever actually
    looked at the .rtl output to test this.


r~


	* i386.h (PREFERRED_RELOAD_CLASS): i387 constants get FP_TOP_REG.
	* i386.md (movsf, movdf, movxf): Call validize_mem to clean up
	force_const_mem output for -fpic.  Do not try to emit REG_NOTES.
	During reload, fix up not-quite-correct constant pool addresses.
    

Index: gcc/config/i386/i386.h
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/config/i386/i386.h,v
retrieving revision 1.31
diff -c -p -d -u -r1.31 i386.h
--- i386.h	1998/08/25 12:23:49	1.31
+++ i386.h	1998/09/03 22:54:34
@@ -932,7 +932,8 @@ enum reg_class
    movdf to do mem-to-mem moves through integer regs. */
 
 #define PREFERRED_RELOAD_CLASS(X,CLASS)	\
-  (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode ? NO_REGS	\
+  (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode \
+   ? (standard_80387_constant_p (X) ? FP_TOP_REG : NO_REGS) \
    : GET_MODE (X) == QImode && ! reg_class_subset_p (CLASS, Q_REGS) ? Q_REGS \
    : ((CLASS) == ALL_REGS						\
       && GET_MODE_CLASS (GET_MODE (X)) == MODE_FLOAT) ? GENERAL_REGS	\
Index: gcc/config/i386/i386.md
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/config/i386/i386.md,v
retrieving revision 1.34
diff -c -p -d -u -r1.34 i386.md
--- i386.md	1998/08/25 12:23:51	1.34
+++ i386.md	1998/09/03 22:54:35
@@ -1234,27 +1234,32 @@
     }
 
   /* If we are loading a floating point constant that isn't 0 or 1
-     into a register, indicate we need the pic register loaded.  This could
-     be optimized into stores of constants if the target eventually moves
-     to memory, but better safe than sorry.  */
+     into a register, force the value to memory now, since we'll 
+     get better code out the back end.  */
   else if ((reload_in_progress | reload_completed) == 0
-      && GET_CODE (operands[0]) != MEM
-      && GET_CODE (operands[1]) == CONST_DOUBLE
-      && !standard_80387_constant_p (operands[1]))
+	   && GET_CODE (operands[0]) != MEM
+	   && GET_CODE (operands[1]) == CONST_DOUBLE
+	   && !standard_80387_constant_p (operands[1]))
     {
-      rtx insn, note, fp_const;
+      operands[1] = validize_mem (force_const_mem (SFmode, operands[1]));
+    }
 
-      fp_const = force_const_mem (SFmode, operands[1]);
-      if (flag_pic)
-	current_function_uses_pic_offset_table = 1;
+  /* Most of the time someone else should have taken care that addresses
+     are valid.  But reload assumes it can always just load from a symbol,
+     which we need to frob into a @GOTOFF reference.  */
+  else if (reload_in_progress && flag_pic
+	   && GET_CODE (operands[0]) == MEM
+	   && GET_CODE (XEXP (operands[0], 0)) == SYMBOL_REF
+	   && CONSTANT_POOL_ADDRESS_P (XEXP (operands[0], 0)))
+    {
+      rtx new;
 
-      insn = emit_insn (gen_rtx_SET (SFmode, operands[0], fp_const));
-      note = find_reg_note (insn, REG_EQUAL, NULL_RTX);
+      current_function_uses_pic_offset_table = 1;
+      new = gen_rtx_UNSPEC (VOIDmode, gen_rtvec (1, XEXP (operands[1], 0)), 7);
+      new = gen_rtx_CONST (VOIDmode, new);
+      new = gen_rtx_PLUS (Pmode, pic_offset_table_rtx, new);
 
-      if (note)
-	XEXP (note, 0) = operands[1];
-      else
-	REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_EQUAL, operands[1], REG_NOTES (insn));
+      operands[1] = change_address (operands[1], VOIDmode, new);
     }
 }")
 
@@ -1330,6 +1335,7 @@
     return AS1 (fxch,%0);
 }")
 
+
 (define_insn "movdf_push"
   [(set (match_operand:DF 0 "push_operand" "=<,<")
 	(match_operand:DF 1 "general_operand" "*rfF,o"))]
@@ -1386,23 +1392,29 @@
      optimized into stores of constants if the target eventually moves to
      memory, but better safe than sorry.  */
   else if ((reload_in_progress | reload_completed) == 0
-      && GET_CODE (operands[0]) != MEM
-      && GET_CODE (operands[1]) == CONST_DOUBLE
-      && !standard_80387_constant_p (operands[1]))
+	   && GET_CODE (operands[0]) != MEM
+	   && GET_CODE (operands[1]) == CONST_DOUBLE
+	   && !standard_80387_constant_p (operands[1]))
     {
-      rtx insn, note, fp_const;
+      operands[1] = validize_mem (force_const_mem (DFmode, operands[1]));
+    }
 
-      fp_const = force_const_mem (DFmode, operands[1]);
-      if (flag_pic)
-	current_function_uses_pic_offset_table = 1;
+  /* Most of the time someone else should have taken care that addresses
+     are valid.  But reload assumes it can always just load from a symbol,
+     which we need to frob into a @GOTOFF reference.  */
+  else if (reload_in_progress && flag_pic
+	   && GET_CODE (operands[0]) == MEM
+	   && GET_CODE (XEXP (operands[0], 0)) == SYMBOL_REF
+	   && CONSTANT_POOL_ADDRESS_P (XEXP (operands[0], 0)))
+    {
+      rtx new;
 
-      insn = emit_insn (gen_rtx_SET (DFmode, operands[0], fp_const));
-      note = find_reg_note (insn, REG_EQUAL, NULL_RTX);
+      current_function_uses_pic_offset_table = 1;
+      new = gen_rtx_UNSPEC (VOIDmode, gen_rtvec (1, XEXP (operands[1], 0)), 7);
+      new = gen_rtx_CONST (VOIDmode, new);
+      new = gen_rtx_PLUS (Pmode, pic_offset_table_rtx, new);
 
-      if (note)
-	XEXP (note, 0) = operands[1];
-      else
-	REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_EQUAL, operands[1], REG_NOTES (insn));
+      operands[1] = change_address (operands[1], VOIDmode, new);
     }
 }")
 
@@ -1535,23 +1547,29 @@
      be optimized into stores of constants if the target eventually moves
      to memory, but better safe than sorry.  */
   else if ((reload_in_progress | reload_completed) == 0
-      && GET_CODE (operands[0]) != MEM
-      && GET_CODE (operands[1]) == CONST_DOUBLE
-      && !standard_80387_constant_p (operands[1]))
+	   && GET_CODE (operands[0]) != MEM
+	   && GET_CODE (operands[1]) == CONST_DOUBLE
+	   && !standard_80387_constant_p (operands[1]))
     {
-      rtx insn, note, fp_const;
+      operands[1] = validize_mem (force_const_mem (XFmode, operands[1]));
+    }
 
-      fp_const = force_const_mem (XFmode, operands[1]);
-      if (flag_pic)
-	current_function_uses_pic_offset_table = 1;
+  /* Most of the time someone else should have taken care that addresses
+     are valid.  But reload assumes it can always just load from a symbol,
+     which we need to frob into a @GOTOFF reference.  */
+  else if (reload_in_progress && flag_pic
+	   && GET_CODE (operands[0]) == MEM
+	   && GET_CODE (XEXP (operands[0], 0)) == SYMBOL_REF
+	   && CONSTANT_POOL_ADDRESS_P (XEXP (operands[0], 0)))
+    {
+      rtx new;
 
-      insn = emit_insn (gen_rtx_SET (XFmode, operands[0], fp_const));
-      note = find_reg_note (insn, REG_EQUAL, NULL_RTX);
+      current_function_uses_pic_offset_table = 1;
+      new = gen_rtx_UNSPEC (VOIDmode, gen_rtvec (1, XEXP (operands[1], 0)), 7);
+      new = gen_rtx_CONST (VOIDmode, new);
+      new = gen_rtx_PLUS (Pmode, pic_offset_table_rtx, new);
 
-      if (note)
-	XEXP (note, 0) = operands[1];
-      else
-	REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_EQUAL, operands[1], REG_NOTES (insn));
+      operands[1] = change_address (operands[1], VOIDmode, new);
     }
 }")
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]