Thanks for the updates.
Vladimir Makarov <vmaka...@redhat.com> writes:
>>> + a change in the offset between the eliminable register and its
>>> + substitution if UPDATE_P, or the full offset if FULL_P, or
>>> + otherwise zero.
>> I wonder if an enum would be better than two booleans?
>> It avoids invalid combinations like UPDATE_P && FULL_P
>> and might make the arguments more obvious too.
> IMHO, It is matter of choice. I don't like to introduce a new enum just
> for the function. It is pretty standard situation. I usually introduce
> enum when there are a few combinations prohibited.
OK. I agree this is probably personal preference.
>>> + /* The only time we want to replace a PLUS with a REG
>>> + (this occurs when the constant operand of the PLUS is
>>> + the negative of the offset) is when we are inside a
>>> + MEM. We won't want to do so at other times because
>>> + that would change the structure of the insn in a way
>>> + that reload can't handle. We special-case the
>>> + commonest situation in eliminate_regs_in_insn, so
>>> + just replace a PLUS with a PLUS here, unless inside a
>>> + MEM. */
>> Reload reference. Does this restriction still apply? The later comment:
> I don't think so. I removed the comment.
Well, the question was about the code as much as the comment.
The comment did describe what the code did:
if (mem_mode != 0
&& CONST_INT_P (XEXP (x, 1))
&& INTVAL (XEXP (x, 1)) == -offset)
return to;
else
return gen_rtx_PLUS (Pmode, to,
plus_constant (Pmode,
XEXP (x, 1), offset));
If the restriction doesn't apply any more then the mem_mode condition
should be removed. If does apply then we should have some sort of
comment to explain why.
I suppose the question is: what happens for PLUS match_operators?
If elimination changes a (plus (reg X) (const_int Y)) into (reg X'),
and the (plus ...) is matched via a match_operator, will LRA cope
correctly? Or does LRA require a plus matched via a match_operator
to remain a plus? Or shouldn't we eliminate match_operators at all,
and just process true operands?
I wasn't sure at this point (and still haven't read through everything,
so am still not sure now).
>>> + Note that there is no risk of modifying the structure of the insn,
>>> + since we only get called for its operands, thus we are either
>>> + modifying the address inside a MEM, or something like an address
>>> + operand of a load-address insn. */
> I removed this too.
I think that's still accurate and should be kept. I was just using
it to emphasise a point (probably badly, sorry).
>> makes it sound on face value like the MEM restriction above is a
>> reload-specific
>> thing. Same question for:
>>
>>> + /* As above, if we are not inside a MEM we do not want to
>>> + turn a PLUS into something else. We might try to do so here
>>> + for an addition of 0 if we aren't optimizing. */
It looks like your follow-up patch left this alone FWIW.
>>> +#ifdef WORD_REGISTER_OPERATIONS
>>> + /* On these machines, combine can create RTL of the form
>>> + (set (subreg:m1 (reg:m2 R) 0) ...)
>>> + where m1 < m2, and expects something interesting to
>>> + happen to the entire word. Moreover, it will use the
>>> + (reg:m2 R) later, expecting all bits to be preserved.
>>> + So if the number of words is the same, preserve the
>>> + subreg so that push_reload can see it. */
>>> + && ! ((x_size - 1) / UNITS_PER_WORD
>>> + == (new_size -1 ) / UNITS_PER_WORD)
>>> +#endif
>> Reload reference (push_reload). Do we still need this for LRA?
> It is hard me to say. So I would not touch this code at least for now.
> I changed push reload to LRA.
Could I ask you to reconsider? The situation that the comment describes
sounds like a bug to me. Removing it shouldn't affect the 4.8 submission.
It just seems to me that LRA is our big chance of getting rid of some
of this cruft. If we're too scared to touch code like this even on
a big change like reload to LRA, we'll never be able to touch it.
>>> +static void
>>> +eliminate_regs_in_insn (rtx insn, bool replace_p)
>>> +{
>>> + int icode = recog_memoized (insn);
>>> + rtx old_set = single_set (insn);
>>> + bool val;
>> "validate_p" might be a better name.
>>
> Done.
Sorry for being too terse. I see you've changed "replace_p" to
"validate_p", but I actually meant that _"val"_ should be changed
to "validate_p". Elsewhere you use "val" to mean "value number",
and "val" could equally mean "validate_p" or "validated_p".
"replace_p" was a good name. :-)
>>> + /* If an output operand changed from a REG to a MEM and INSN is an
>>> + insn, write a CLOBBER insn. */
>>> + if (static_id->operand[i].type != OP_IN
>>> + && REG_P (orig_operand[i])
>>> + && MEM_P (substed_operand[i])
>>> + && replace_p)
>>> + emit_insn_after (gen_clobber (orig_operand[i]), insn);
>> I realise this is copied from reload, but why is it needed?
> I did minimal changes to original code. I'll check it on bootstrap and
> if it is ok, I'll remove it.
Thanks.
Richard
