Re: Problem on i386 with -fpic -fomit-frame-pointer

[Bernd Schmidt <crux at pool dot informatik dot rwth-aachen dot de>]

>   > With the reload patch I sent in on Monday, gcc miscompiled the file e_hypot
>   > l.c
>   > from glibc-2.0.95. The problem appears to be in i386.md, in the mov[sdx]f
>   > patterns. I believe the following change is incomplete:
>   > 
>   > Sun Jul 26 01:11:12 1998  H.J. Lu  (hjl@gnu.org)
>   > 
>   >         * i386.h (CONST_DOUBLE_OK_FOR_LETTER_P): Return 0 when eliminating
>   >         the frame pointer and compiling PIC code and reload has not complet
>   > ed.
>   > 
>   > If using -fomit-frame-pointer together with -fPIC, this will not accept any
>   > floating point constants, and reload will substitute memory references for
>   > them.  However, the expander patterns have not been updated together with
>   > this patch, they still assume that certain constants are valid.  The result
>   > was that in the file that was miscompiled, a load instruction using the PIC
>   > register was emitted, but during reload the variable
>   > current_function_uses_pic_offset_table remains set to zero. This causes
>   > incorrect register elimination offsets, which in turn causes some of the
>   > generated store instructions to write data outside the stack frame and
>   > clobber one of the callee-saved registers.
>   > 
>   > Bernd
>   > 
>   > 
>   > 	* i386.md (movsf, movdf, movxf): Accept no constants if -fPIC and
>   > 	-fomit-frame-pointer.
> Did this problem get fixed?  I know rth did some work on the movsf, movdf and
> movxf patterns for the x86, but I don't know if they were supposed to solve
> this problem or not.

I'm not entirely certain about what is going on.  We are definitely having
some weird problems still.  First of all, I'd like to see the original test
case that led to the Jul 26 change, if someone still has it.

These are the current definitions which are relevant to the issue.  HJ's
change modified CONST_DOUBLE_OK_FOR_LETTER_P, and was followed by
several changes by Richard Henderson and me in PREFERRED_RELOAD_CLASS.

============
/* Similar, but for floating constants, and defining letters G and H.
   Here VALUE is the CONST_DOUBLE rtx itself.  We allow constants even if
   TARGET_387 isn't set, because the stack register converter may need to
   load 0.0 into the function value register.

   We disallow these constants when -fomit-frame-pointer and compiling
   PIC code since reload might need to force the constant to memory.
   Forcing the constant to memory changes the elimination offsets after
   the point where they must stay constant.

   However, we must allow them after reload as completed as reg-stack.c
   will create insns which use these constants.  */

#define CONST_DOUBLE_OK_FOR_LETTER_P(VALUE, C)  \
  (((reload_completed || !flag_pic || !flag_omit_frame_pointer) && (C) == 'G') \
   ? standard_80387_constant_p (VALUE) : 0)

/* Given an rtx X being reloaded into a reg required to be
   in class CLASS, return the class of reg to actually use.
   In general this is just CLASS; but on some machines
   in some cases it is preferable to use a more restrictive class.
   On the 80386 series, we prevent floating constants from being
   reloaded into floating registers (since no move-insn can do that)
   and we ensure that QImodes aren't reloaded into the esi or edi reg.  */

/* Put float CONST_DOUBLE in the constant pool instead of fp regs.
   QImode must go into class Q_REGS.
   Narrow ALL_REGS to GENERAL_REGS.  This supports allowing movsf and
   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		\
   ? (standard_80387_constant_p (X)					\
      ? reg_class_subset_p (CLASS, FLOAT_REGS) ? CLASS : FLOAT_REGS	\
      : 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	\
   : (CLASS))
============

We are currently having problems with the code in reload1.c that checks
reload insns which were emitted for asm statements for validity.  The
CONST_DOUBLE_OK_FOR_LETTER_P macro will reject standard 387 constants at
the time, because reload_completed is still zero, and we'll abort
occasionally for (perfectly valid) reload insns which load constant
values into a FLOAT_REG.  These didn't occur previously, presumably
because until recently, PREFERRED_RELOAD_CLASS returned VOIDmode for all
CONST_DOUBLES.
That we're now creating such insns might in fact be good news, because
that means we might be able to undo HJ's change: if reload no longer
needs to put standard 387 constants into the constant pool, the original
motivation for the change is probably gone.  But I'm not sure, since I
don't have the original test case.

Bernd