[Bug rtl-optimization/24194] New: emit_input_reload_insns secondary reload handling is unsafe

[Bug rtl-optimization/24194] New: emit_input_reload_insns secondary reload handling is unsafe

[amylaar at gcc dot gnu dot org]

When emit_input_reload_insns has an oldequiv that is different from old,
it re-computes the reload pattern icode used for secondary reloads.
It checks the predicate of operand 0 against the relod register, something
md.texi says doesn't happen and it clears icode for a mismatch.  Instead it
should check the constraint of operand 0 against the reload register, and
abandom the inheritance in case of a mismatch (since then we'd need another
temporary register).

Moreover, it assumes that the class returned by SECONDARY_RELOAD_CLASS will be
suitable for the scratch register.  The tm.texi description of
SECONDARY_INPUT_RELOAD_CLASS seems a bit ambigous:

 "Specifically, if copying @var{x} to a
register @var{class} in @var{mode} requires an intermediate register,
you should define @code{SECONDARY_INPUT_RELOAD_CLASS} to return the
largest register class all of whose registers can be used as
intermediate registers or scratch registers."

If the returned class contains registers that might be used for one, but
not necessarily the other reload, the definition is is usually possible
(if not, that can be archived by adding another union class), but the above
assumption by emit_input_reload_insns is unsafe.
If you interpret it as the largest class so that either all the registers
are suitable as an intermediate register, or so that all of the registers are
suitable as scratch registers, then the definition might be impossible when
the two reloads allow maximal classes of equal size, and when it is possible,
and emit_input_reload_insns is unsafe as above.
If you interpret the definition so that each register must be suitable for
either reload, then the assumption made by emit_input_reload_insns about
the suitability of this register is safe, but the definition of the macro
can be impossible when the largest class to fit the description is NO_REGS.
In fact, if a union class exists, that should be suitable as a scratch register
which can also be used internally as a temp register, so no strict need to
use a tertiary reload arises in the first place.

But the worst problem is what happens when the icode found for real_oldequiv
needs a scratch register a scratch register with a larger mode than the
original one.  There is no check to avoid using more hard registers than were
allocated in the first place, so that the secondary reload might clobber
additional registers.

I'm not sure if we should add the wrong-code keyword for this problem; the
potential for wrong code is there, but we know of no actual instance of
wrong code generation, nor have we proven that there exists one for any of
the existing ports.


-- 
           Summary: emit_input_reload_insns secondary reload handling is
                    unsafe
           Product: gcc
           Version: 4.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: rtl-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: amylaar at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24194

[Bug rtl-optimization/24194] emit_input_reload_insns secondary reload handling is unsafe

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2006-07-20 11:19 -------
Isn't this really fixed by the TARGET_SECONDARY_RELOAD target hook?


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pinskia at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24194

[Bug rtl-optimization/24194] emit_input_reload_insns secondary reload handling is unsafe

[amylaar at spamcop dot net]

------- Comment #2 from amylaar at spamcop dot net  2006-07-20 17:57 -------
Subject: Re:  emit_input_reload_insns secondary
        reload handling is unsafe

Quoting pinskia at gcc dot gnu dot org <gcc-bugzilla@gcc.gnu.org>:

>
>
> ------- Comment #1 from pinskia at gcc dot gnu dot org  2006-07-20 11:19
> -------
> Isn't this really fixed by the TARGET_SECONDARY_RELOAD target hook?

reload_adjust_reg_for_temp checks only the first hard register for suitability
wrt. NEW_CLASS.
I.e. we check TEST_HARD_REG_BIT (reg_class_contents[(int) new_class], regno) ,
but for a two-hard-reg register we'd also have to check
TEST_HARD_REG_BIT (reg_class_contents[(int) new_class], regno) .

AFAICT the other issues in this PR have been addresssed.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24194