Thanks for the updates.
Vladimir Makarov<vmakarov@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).
+#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.