Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> I'm not seriously submitting this patch for approval. I just thought
> it'd be interesting to some people, at least those maintaining ports
> still using reload; I know it's reload and major ports don't really
> care about that anymore. TL;DR: scroll down for the mystery part.
>
> The *benevolent* effects of this bug had me confused for a while: I
> thought I'd see neutral or good effects of nominally *improving* the
> register-class topology of the CRIS port such that there's "a class
> for the union of two classes whenever some instruction allows both
> classes". Essentially, IRA picked (still does, not committed yet) two
> classes GENNONACR_REGS and SPECIAL_REGS and SPEC_GENNONACR_REGS, their
> union, but the move-pattern alternatives corresponding to
> GENNONACR_REGS are actually "r" (GENERAL_REGS), which also contains
> the ACR register. I want to get rid of all things related to ACR and
> simplify the port, as ACR is an artefact of a no-longer-supported CRIS
> variant (see commit r11-220 which also mentions this remaining
> artefact).
>
> But, when improving the topology, instead of still making good use of
> registers from SPECIAL_REGS where the reload pass matters, registers
> from GENERAL_REGS are always preferred. Though, for coremark, this
> only happens in two printf-formatting functions of newlib; _vfprintf_r
> and _vfiprintf_r (same code actually, and it's likely a bug that both
> instances get included).
>
> Anyway, skipping to the point: the effect of the bug is that
> alternatives corresponding to classes that are *not subclasses* of the
> preferred class (the union), are demoted by 4 (or actually: 2 + 2 *
> pref_or_nothing[i]), instead of those register alternatives where the
> register-class *intersection is empty*, as is the intent according to
> my reading of the comment. For CRIS, this means that GENERAL_REGS get
> demoted (as ACR is present there but not in SPEC_GENNONACR_REGS) and
> then SPECIAL_REGS are preferred, which is *sometimes* better as those
> registers are "free". They're not used in regular code and outside
> their special purposes, basically just usable for moving around data;
> perfect for plain single-use spill-reloads. Net result: for CRIS the
> bug causes a slightly smaller coremark binary but the same execution
> time.
>
> With the CRIS register-class topology fixed and the bug no longer
> having effect, I have to adjust the port such that SPECIAL_REGS are
> preferred when that helps (some heuristics required), as the (current)
> order of alternatives for the *movsi_internal pattern causes
> preference of GENERAL_REGS over SPECIAL_REGS when the cost is the
> same. IOW, a flaw in the port masked by a bug in reload.
>
> The mystery isn't so much that there's code mismatching comments or
> intent, but that this code has been there "forever". There has been a
> function reg_classes_intersect_p, in gcc since r0-54, even *before*
> there was reload.c; r0-361, so why the "we don't have a way of forming
> the intersection" in the actual patch and why wasn't this fixed
> (perhaps not even noticed) when reload was state-of-the-art?
But doesn't the comment mean that we have/had no way of getting
a *class* that is the intersection of preferred_class[i] and
this_alternative[i], to store as the new this_alternative[i]?
If the alternatives were register sets rather than classes,
I think the intended effect would be:
this_alternative[i] &= preferred_class[i];
(i.e. AND_HARD_REG_SET in old money).
It looks like the patch would instead make this_alternative[i] include
registers that the alternative doesn't actually accept, which feels odd.
Thanks,
Richard
> (I know the file has been renamed but again this isn't an patch
> intended to be applied. The context is adjusted to include the
> comment. The ChangeLog entry is the effect of a habitual twitch. 8-]
> )
>
> gcc:
> * reload.c (find_reloads): Call reg_classes_intersect_p, not
> reg_class_subset_p.
> ---
> gcc/reload.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/reload.c b/gcc/reload.c
> index 4c55ca58a5f7..36e887c6b57e 100644
> --- a/gcc/reload.c
> +++ b/gcc/reload.c
> @@ -3616,43 +3616,39 @@ find_reloads (rtx_insn *insn, int replace, int
> ind_levels, int live_known,
> /* If this operand is a pseudo register that didn't get
> a hard reg and this alternative accepts some
> register, see if the class that we want is a subset
> of the preferred class for this register. If not,
> but it intersects that class, use the preferred class
> instead. If it does not intersect the preferred
> class, show that usage of this alternative should be
> discouraged; it will be discouraged more still if the
> register is `preferred or nothing'. We do this
> because it increases the chance of reusing our spill
> register in a later insn and avoiding a pair of
> memory stores and loads.
>
> Don't bother with this if this alternative will
> accept this operand.
>
> Don't do this for a multiword operand, since it is
> only a small win and has the risk of requiring more
> spill registers, which could cause a large loss.
>
> Don't do this if the preferred class has only one
> register because we might otherwise exhaust the
> class. */
>
> if (! win && ! did_match
> && this_alternative[i] != NO_REGS
> && known_le (GET_MODE_SIZE (operand_mode[i]), UNITS_PER_WORD)
> && reg_class_size [(int) preferred_class[i]] > 0
> && ! small_register_class_p (preferred_class[i]))
> {
> if (! reg_class_subset_p (this_alternative[i],
> preferred_class[i]))
> {
> - /* Since we don't have a way of forming the intersection,
> - we just do something special if the preferred class
> - is a subset of the class we have; that's the most
> - common case anyway. */
> - if (reg_class_subset_p (preferred_class[i],
> - this_alternative[i]))
> + if (reg_classes_intersect_p (preferred_class[i],
> + this_alternative[i]))
> this_alternative[i] = preferred_class[i];
> else
> reject += (2 + 2 * pref_or_nothing[i]);
> }
