Re: [RFA] CONSTANT_RTX_P patch to builtins.c

[Roger Sayle <roger at eyesopen dot com>]

On Thu, 16 Oct 2003, Eric Christopher wrote:
> I was looking into the bcp-1.c failure on mipsisa64-elf and came across
> this. Conveniently it happens to fix the failure too - and I'll admit
> now that I'm not positive what it does to compile times. :) Then again,
> we're already looping over every insn so...
>
> Anyhow, I believe that for_each_rtx is the proper way to loop over the
> insns: 1) so we won't miss anything, and 2) i believe it's safe to
> remove any qualifiers like sign/zero_extend, subreg from it. It also has
> the side effect of being a lot cleaner.
>
> Roger: You and I had already spoken about this - what do you think here?
>
> Thoughts?


I personally don't want to reject or approve this patch, which I'll
leave to authorities higher than myself.  However, I describe how it
is that I envision things should be working.

In my ideal world, a CONSTANT_P_RTX node should only ever appear at the
top-level in the SET_SRC of a single_set instruction.  This can be
enforced by ensuring that the only pattern that allows constant_p_rtx
as an operand is a movsi2 or similar.  All other backend patterns should
reject CONSTANT_P_RTX as an operand.

My motivations for this approach are:

[1] Performance.  As you mention above looping through every RTL node
    in an instruction can be considerably slower than just touching
    every insn.  Indeed, as I've explained already, I submitted three
    follow-up patches to try and minimize the 1% slow-down caused by
    purge_builtin_constant_p.

[2] Consistency.  Currently all but two targets already implement these
    semantics.  The only two targets that don't are mipsisa64-*-*
    and h8300*-*-*.  the first where mips.md allow constant_p_rtx to
    leak inside sign_extend inside its extendsidi2 expander.  The
    second where the h8300 lets these things leak into subregs.
    This patch slows down all other targets just to work around issues
    with these two.

[3] Back-end correctness.  The leaking of constant_p_rtx into these
    insn patterns, commonly reflects a "bug" in the way the target's
    operand constraint checking is coded.  Often they're using the
    macro CONSTANT_P instead of checking the structure of the RTL
    itself, i.e. CONST_INT, etc...  Not only does this create problems
    for constant_p_rtx, but it will also match arbitrary CONST_RTX
    nodes, such as "(const (neg (const (plus X Y)))" and other
    arbitrarily complex RTL expressions that the optimizers mark as
    CONST.  See http://gcc.gnu.org/ml/gcc-patches/2003-09/msg01664.html

[4] Middle-end correctness.  Both the current and patched versions
    of purge_builtin_constant_p assume that it is safe to replace
    the constant_p_rtx with a suitable CONST_INT without having to
    re-recognize the instruction.  Generally, this is only safe for
    simple mov patterns where that alternative must be provided by
    the backend.  Generally, its unsafe to replace/substitute inside
    an arbitrary instruction.  For example, Alex's fix to substitute
    into SUBREGs could have caused problems determining the inner-mode
    of the SUBREG (as the CONST_INT doesn't have a mode) if it weren't
    for the fact that the entire SUBREG was replaced.

    On a related issue this is another bug in your patch.  You scan
    the entire RTL looking for constant_p_rtx, which could appear
    under a binary operator, in an unspec, etc... but no matter where
    the constant_p_rtx note is found, you replace the entire SET_SRC.


Implicit in this code is also the assumption that constant_p_rtx
can't occur in register notes or instructions other than single_sets,
and implicitly with your patch only under extension and truncation
operators, so it isn't a huge leap to require them be strictly limited
to simple moves.


The correct fix as I see it is to tighten up the constraint matching
code in mips.c and h8300.c, to match the other GCC backends, and then
simplify purge_builtin_constant_p back to its original form by
reverting http://gcc.gnu.org/ml/gcc-patches/2003-02/msg01969.html


But those are just my opinions, and I wouldn't want them to block
your fix.  If you are going to look deeper inside the insn's pattern
then I agree for_each_rtx is the way to go, but as I explained above
you may need to rerecognize the modified instruction.


I'm really new to this reviewing business, so I'd *very* much appreciate
it if someone with more experience could comment on whether my concerns
are justified or not.

Alas I'm not very good with backends but Eric's solution might be to
either exclude constant_p_rtx from mips.c's move_operand, or just add
an additional constraint of this single define_expand that "GET_CODE
(operands[1]) != CONSTANT_P_RTX".


I will however admit that the issue is my fault.  When I originally
wrote purge_builtin_constant_p, I assumed, by design, that these node
could only occur at the top-level, however I never documented that as
a constraint or requirement on a backend.  It just seemed like an
invariant that every backend already guaranteed.  Sorry.

Roger
--