On 10/19/2018 05:16 PM, Peter Bergner wrote:
Vlad, Jeff and Segher,
I think I have determined what is happening with the aarch64 test case that
is failing after my r264897 commit. It appears my patch is just exposing
an issue in lra-constraints.c:process_alt_operands() when processing an insn
with early clobber operands. Jeff & Segher, I have reverted all of my
previous patches we were making to lra-lives.c for the analysis below and
am just using straight trunk revision 264897 (ie, my last patch).
...
Secondly, I don't think it's ever legal to reload an operand
that has been hard coded by the user to a hard register via a register asm
definition like in the test case above.
Yes, if a variable declared as an asm variable, the asm insn should use
its hard register. That is what people expect and that is what the GCC
documentation about asm says. In most cases, reload of such asm variable
hard registers do not happen as they satisfy the constraints except for
this complicated earlyclobber case you found.
With that in mind, what do people think of the patch below? This fixes the
AARCH64 test case. However, it ICE's on the following test case:
long foo (long arg)
{
register long var asm("x0");
asm("bla %0 %1" : "+&r"(var) : "r"(arg));
return var;
}
...but that is due to a combine bug where combine replaces the use of
"arg"'s pseudo in the inline asm with the incoming argument reg x0 which
should be illegal. Ie, it's taking a valid inline asm and creating an
invalid one since the earlyclobber op and non-matching op have both
been forced to use the same hard reg. Segher has a combine patch to
stop that which he is going to submit (commit?) soon. With my patch,
we are also now able to catch the following user error, when before we
could not:
I think usage of hard registers in RTL should be minimal. Any their
propagation besides RA creates a risk for RA work and probably does not
improve the final code.
...
Thoughts? I'll note that this does not fix the S390 bugs, since those seem
to be due to problems with early clobber operands and "matching" constraint
operands. I'm still working on that and hope to have something soon.
I think your initial patch triggered a bug in RA. Your patch looks a
reasonable solution for me. But the patch touches a very sensitive code
in LRA (I don't remember when a change in
lra-constraints.c:process_alt_operands worked completely fine at the
first iteration). So it would be nice to test it on many targets before
committing.
Peter, thank you for continuing your work on these RA issues.
I am currently bootstrapping this on s390x just to make sure I don't break
anything there, before debugging the existing s390x issue(s). I can't test
this on aarch64 or arm, other than knowing my aarch64 cross doesn't ICE on
the test case above.
Peter
* lra-constraints.c (process_alt_operands): Abort on illegal hard
register usage. Prefer reloading non hard register operands.
Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c (revision 264897)
+++ gcc/lra-constraints.c (working copy)
@@ -2904,18 +2904,29 @@ process_alt_operands (int only_alternati
if (first_conflict_j < 0)
first_conflict_j = j;
last_conflict_j = j;
+ /* Both the earlyclobber operand and conflicting operand
+ cannot both be hard registers. */
+ if (REGNO (operand_reg[i]) < FIRST_PSEUDO_REGISTER
+ && operand_reg[j] != NULL_RTX
+ && REGNO (operand_reg[j]) < FIRST_PSEUDO_REGISTER)
+ fatal_insn ("unable to generate reloads for:", curr_insn);
}
if (last_conflict_j < 0)
continue;
- /* If earlyclobber operand conflicts with another
- non-matching operand which is actually the same register
- as the earlyclobber operand, it is better to reload the
- another operand as an operand matching the earlyclobber
- operand can be also the same. */
- if (first_conflict_j == last_conflict_j
- && operand_reg[last_conflict_j] != NULL_RTX
- && ! curr_alt_match_win[last_conflict_j]
- && REGNO (operand_reg[i]) == REGNO (operand_reg[last_conflict_j]))
+
+ /* If an earlyclobber operand conflicts with another non-matching
+ operand (ie, they have been assigned the same hard register),
+ then it is better to reload the other operand, as there may
+ exist yet another operand with a matching constraint associated
+ with the earlyclobber operand. However, if one of the operands
+ is an explicit use of a hard register, then we must reload the
+ other non-hard register operand. */
+ if (REGNO (operand_reg[i]) < FIRST_PSEUDO_REGISTER
+ || (operand_reg[last_conflict_j] != NULL_RTX
+ && REGNO (operand_reg[last_conflict_j])
+ >= FIRST_PSEUDO_REGISTER
+ && first_conflict_j == last_conflict_j
+ && ! curr_alt_match_win[last_conflict_j]))
{
curr_alt_win[last_conflict_j] = false;
curr_alt_dont_inherit_ops[curr_alt_dont_inherit_ops_num++]
