Re: Use simplify_replace_rtx rather than wrap_constant

[Richard Sandiford <rdsandiford at googlemail dot com>]

Bernd Schmidt <bernds_cb1@t-online.de> writes:
> Richard Sandiford wrote:
>> I made a couple of other changes to simplify_replace_rtx too:
>> 
>>   - It now copies the "to" rtx for each replacement.  All but one caller
>>     seemed to pass an already-used rtx as the "to" value, so current
>>     substitutions could create invalid sharing.  I've removed the
>>     now-redundant copy_rtx from the one caller that used it.
>> 
>>     (Doing the copy lazily seems better, and copes with cases where the
>>     value is used twice within the same expression.)
>> 
>>   - It now uses rtx_equal_p to check for equivalence.  Some callers seem
>>     to pass MEM rtxes, which can't be shared, so I think using rtx_equal_p
>>     is better than relying on pointer equivalence for everything except REGs.
>>     I scanned the current callers, and none of them seemed to be using
>>     the function in a way that genuinely required strict pointer
>>     equivalence.
>> 
>>     rtx_equal_p isn't cheap of course.  We could reduce the overhead
>>     by turning it into an inline wrapper that first checks whether the
>>     code and mode are the same (or maybe just the code) and only then
>>     calls an out-of-line function.
>
> Can you split these out?  They seem to be unrelated to the problem at
> hand and I'm not sure they are good changes.  For the
> simplify_replace_rtx cases in loop_iv.c, for example, I'm quite sure
> invalid sharing is not an issue.

The combine.c code isn't right without these two changes.
The current combine code (rightly IMO) uses rtx_equal_p to
check for equivalence between the rtx and the "from" value,
and it uses copy_rtx to make sure that each "to" value is unique.

Why don't you think the changes are correct?  If the "from"
value occurs twice in the expression then we'll end up with
two copies of the "to" value.  We ought to make sure that
the two copies are only shared if copy_rtx says that they
_can_ be shared.  Callers in general don't know whether the
the "from" expression only occurs once, or whether the "to"
value can be shared, so I don't think the onus should be
on them.

Likewise with rtx_equal_p.  The reason we prohibit sharing of MEMs is
(AIUI) because we want to be able to perform substitutions on one MEM's
address without it having a knock-on effect in some unrelated situation.
The different addresses don't carry any semantic information.  So I
think using rtx_equal_p should be the default position, and we should
only switch to pointer equivalence if there is s specific need to, or if
it can be guaranteed correct for all callers.  It doesn't seem
appropriate for such a general function as simplify_replace_rtx.

I think any _reliance_ on pointer equivalence (i.e. any deliberate
_rejection_ of rtx_equal_p for correctness reasons) is special enough
that it ought to be flagged up by something in the function name
(and ideally a comment too).  As I mentioned above, I tried to check
that each current caller of simplify_replace_rtx does not rely on
pointer equivalence in this way.

Do you think the changes are unsafe, or are you worried about the
performance impact?  If the latter, then what about my suggestion
of making rtx_equal_p an inline wrapper to an out-of-line function?
I think that is a separate change, but I'm happy to do it if you
think it's worthwhile (or even necessary for the main patch to
be acceptable).

Richard