x86 PIC regressions
Richard Henderson
rth@cygnus.com
Thu Sep 3 16:33:00 GMT 1998
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);
}
}")
More information about the Gcc-patches
mailing list