On 01/16/2017 10:47 AM, Matthew Fortune wrote:
Hi Vladimir,
I'm working on PR target/78660 which is looking like a latent LRA bug.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660
I believe the problem is in the same area as a bug was fixed in 2015:
https://gcc.gnu.org/ml/gcc-patches/2015-01/msg02165.html
Eric pointed out that the new issue relates to something reload
specifically dealt with in reload1.c:eliminate_regs_1:
if (MEM_P (new_rtx)
&& ((x_size < new_size
/* On RISC 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. */
&& !(WORD_REGISTER_OPERATIONS
&& (x_size - 1) / UNITS_PER_WORD
== (new_size -1 ) / UNITS_PER_WORD))
|| x_size == new_size)
)
return adjust_address_nv (new_rtx, GET_MODE (x), SUBREG_BYTE (x));
else
return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x));
However the code in lra-constraints.c:curr_insn_transform does not appear
to make any attempt to handle a special case for WORD_REGISTER_OPERATIONS.
I tried the following patch to account for this, which 'works' but I'm not
at all sure what the conditions should be (the comment from reload will
need adapting and including as well):
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 260591a..ac8d116 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4086,7 +4086,9 @@ curr_insn_transform (bool check_only_p)
&& (goal_alt[i] == NO_REGS
|| (simplify_subreg_regno
(ira_class_hard_regs[goal_alt[i]][0],
- GET_MODE (reg), byte, mode) >= 0)))))
+ GET_MODE (reg), byte, mode) >= 0)))
+ || (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg))
+ && WORD_REGISTER_OPERATIONS)))
{
if (type == OP_OUT)
type = OP_INOUT;
I think at the very least the issue Richard pointed out in the previous
fix must be dealt with as the new testcase triggers exactly what he
described I believe
Richard Sandiford wrote:
So IMO the patch is too broad. I think it should only use INOUT reloads
for !strict_low if the inner mode is wider than a word and the outer mode
is strictly narrower than the inner mode. That's on top of Vlad's
comment about only converting OP_OUTs, of course.
And here is my attempt at dealing with that:
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index ac8d116..8a0f40f 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4090,7 +4090,17 @@ curr_insn_transform (bool check_only_p)
|| (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg))
&& WORD_REGISTER_OPERATIONS)))
{
- if (type == OP_OUT)
+ /* An OP_INOUT is required when reloading a subreg of a
+ mode wider than a word to ensure that data beyond the
+ word being reloaded is preserved. Also automatically
+ ensure that strict_low_part reloads are made into
+ OP_INOUT which should already be true from the backend
+ constraints. */
+ if (type == OP_OUT
+ && (curr_static_id->operand[i].strict_low
+ || (GET_MODE_SIZE (GET_MODE (reg)) > UNITS_PER_WORD
+ && GET_MODE_SIZE (mode)
+ < GET_MODE_SIZE (GET_MODE (reg)))))
type = OP_INOUT;
loc = &SUBREG_REG (*loc);
mode = GET_MODE (*loc);
Any thoughts on whether this is along the right track would be appreciated.
Mathew, sorry for the delay with the answer. I needed some time to
think about it. Dealing with subregs in lra/reload is a complicated and
sensitive area.
The patch looks ok for me. You can commit it if of course there are no
regressions. I hope the patch will behave well but please, be prepared
to work more on it if there are complications. Sometimes such changes on
LRA need a few iterations.
