Bug 24194 - emit_input_reload_insns secondary reload handling is unsafe
Summary: emit_input_reload_insns secondary reload handling is unsafe
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.1.0
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: 29842
  Show dependency tree / graph
 
Reported: 2005-10-04 18:42 UTC by Jorn Wolfgang Rennecke
Modified: 2016-09-07 08:07 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-01-11 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jorn Wolfgang Rennecke 2005-10-04 18:42:45 UTC
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.
Comment 1 Drea Pinski 2006-07-20 11:19:55 UTC
Isn't this really fixed by the TARGET_SECONDARY_RELOAD target hook?
Comment 2 amylaar@spamcop.net 2006-07-20 17:57:39 UTC
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.
Comment 3 Richard Biener 2012-01-11 14:44:03 UTC
And the remaining one now?