Why does only the SH port need currently_expanding_to_rtl?

Why does only the SH port need currently_expanding_to_rtl?

[Steven Bosscher]

Hi Joern,

Currently we have a hack in cfgexpand, solely for SH, to tell the
backends when we are expanding from trees to RTL.  I added that
hack yesterday as a replacement for rtx_equal_function_value_matters,
which was necessary once for the RTL inliner, but was also being
(ab)used in the ia64 and SH backends to tell if the compiler is
in the process of expanding trees to RTL.  I replaced it with a
global variable set and unset while expanding to RTL, see
currently_expanding_to_rtl.

The code in ia64.c that used rtx_equal_function_value_matters was
not reachable, and that code was removed today.  So now it is only
SH that needs this horrible hack.  I don't understand why, would
you mind explaining it please?  For example, it is used in sh.md
as follows:

(define_insn_and_split "load_ra"
  [(set (match_operand:SI 0 "general_movdst_operand" "")
	(unspec:SI [(match_operand 1 "register_operand" "")] UNSPEC_RA))]
  "TARGET_SH1"
  "#"
  "&& ! currently_expanding_to_rtl"
  [(set (match_dup 0) (match_dup 1))]
  "
{
  if (TARGET_SHCOMPACT && current_function_has_nonlocal_label)
    operands[1] = gen_rtx_MEM (SImode, return_address_pointer_rtx);
}")

See also define_expands "seq", "slt", "sgt", "sge", "sgtu", "sltu",
"sleu", "sgeu", and "sne", and in sh.c four times, three times with
the following comment:

      /* Don't expand fine-grained when combining, because that will
         make the pattern fail.  */
      if (currently_expanding_to_rtl
          || reload_in_progress || reload_completed)

I think it is quite strange that only SH would need this hack, and I
would really like to just get rid of currently_expanding_to_rtl.  Can
you look at this and say if you really need this, and if so, please
explain why?

Gr.
Steven

Re: Why does only the SH port need currently_expanding_to_rtl?

[Steven Bosscher]

On Friday 30 July 2004 01:15, Andreas Schwab wrote:
> Steven Bosscher <stevenb@suse.de> writes:
> > The code in ia64.c that used rtx_equal_function_value_matters was
> > not reachable, and that code was removed today.
>
> Note that it needs to be re-added.

No it doesn't.  If you follow gcc-patches, follow it closely:
http://gcc.gnu.org/ml/gcc-patches/2004-07/msg02630.html

Or be on IRC ;-)

Gr.
Steven

Re: Why does only the SH port need currently_expanding_to_rtl?

[Andreas Schwab]

Steven Bosscher <stevenb@suse.de> writes:

> The code in ia64.c that used rtx_equal_function_value_matters was
> not reachable, and that code was removed today.

Note that it needs to be re-added.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Why does only the SH port need currently_expanding_to_rtl?

[Joern Rennecke]

> The code in ia64.c that used rtx_equal_function_value_matters was
> not reachable, and that code was removed today.  So now it is only
> SH that needs this horrible hack.

Using rtx_equal_function_value_matters to test if rtl is being emitted
was standard practice.  The name was merely a historical accident.

>                                    I don't understand why, would
> you mind explaining it please?  For example, it is used in sh.md
> as follows:
> 
> (define_insn_and_split "load_ra"
>   [(set (match_operand:SI 0 "general_movdst_operand" "")
> 	(unspec:SI [(match_operand 1 "register_operand" "")] UNSPEC_RA))]
>   "TARGET_SH1"
>   "#"
>   "&& ! currently_expanding_to_rtl"
>   [(set (match_dup 0) (match_dup 1))]
>   "
> {
>   if (TARGET_SHCOMPACT && current_function_has_nonlocal_label)
>     operands[1] = gen_rtx_MEM (SImode, return_address_pointer_rtx);
> }")

current_function_has_nonlocal_label is not meaningful before all the
function has been expanded.

> See also define_expands "seq", "slt", "sgt", "sge", "sgtu", "sltu",
> "sleu", "sgeu", and "sne"

I just had a look at the i386.md "seq" pattern and the i386.c
ix86_expand_setcc and ix86_expand_compare functions.
It seems the i386 port would do well to also adopt this 'hack' to avoid
inadvertently clobbering the flags register.  The SETcc expanders
used to be only called at rtl generation time, but these days they can
also be called during if-conversion.  If the flags register is live
across a freshly created SETcc instruction, it will be clobbered.
I suspect that all non-cc0 ports that use a specific hard register for
flags and support SETcc and conditional moves are in the same boat.

>                            and in sh.c four times, three times with
> the following comment:
>
>       /* Don't expand fine-grained when combining, because that will
>          make the pattern fail.  */
>       if (currently_expanding_to_rtl
>           || reload_in_progress || reload_completed)

combiner splitters won't work if they emit more than two instructions.

(! rtx_equal_function_value_matters
 && ! reload_in_progress && ! reload_completed)

is a conservative approximation to 'combine might be running'.
I suspect the ! rtx_equal_function_value_matters term doesn't really
make a difference there, but as stated before, there are other good reasons
why rtx_equal_function_value_matters (by this or any other name) is needed,
and you really need more testing for performance related patches to get
confidence that they are or not doing what they are not supposed to do
than for correctness related ones.  A performance drop of 0.1% is hard to
nail down, but if such a drop happens daily, we'll lose 30% performance
per year.