Hi Peter,
On Fri, Oct 19, 2018 at 04:16:12PM -0500, Peter Bergner wrote:
> In lra-constraints.c:process_alt_operands(), we notice that pseudo 92 is
> assigned to x1 and that an early clobber operand is also assigned to x1, or
> rather, that it uses x1 explicitly. This is enough to trigger reload(s),
> but the problem is we end up trying to reload the early clobber operand
> which has been forced into x1 via register asm assignment, instead of
> pseudo 92 which conflicts with it.
> 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.
Yup.
> 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;
> }
(As an "unable to generate reloads for" fatal).
> ...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.
Yup. It is quite a bit more generic: it prevents combine from combining
a hard-not-fixed-reg-to-pseudo-copy with any instruction, not just with
asm's.
> With my patch,
> we are also now able to catch the following user error, when before we
> could not:
>
> bergner@pike:~/gcc/BUGS/PR87507/AARCH64$ cat ice4.i
> long foo (void)
> {
> register long arg asm("x1");
> register long var asm("x1");
> asm ("bla %0 %1" : "=&r"(arg) : "r"(var));
> return arg;
> }
> bergner@pike:~/gcc/BUGS/PR87507/AARCH64$ .../xgcc -B.../gcc -O2
> -march=armv8.1-a -S ice4.i
> ice4.i: In function âfooâ:
> ice4.i:7:1: error: unable to generate reloads for:
> 7 | }
> | ^
> (insn 5 6 10 2 (set (reg/v:DI 1 x1 [ arg ])
> (asm_operands:DI ("bla %0 %1") ("=&r") 0 [
> (reg/v:DI 1 x1 [ var ])
> ]
> [
> (asm_input:DI ("r") ice4.i:5)
> ]
> [] ice4.i:5)) "ice4.i":5 -1
> (nil))
> during RTL pass: reload
> ice4.i:7:1: internal compiler error: in process_alt_operands, at
> lra-constraints.c:2911
Nice! What did it do before? Oh, it reloaded things into other regs,
generating incorrect code. Ouch!
> I can't test
> this on aarch64 or arm, other than knowing my aarch64 cross doesn't ICE on
> the test case above.
The compile farm has six aarch64 machines ;-)
> --- 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);
You could use HARD_REGISTER_P here, fwiw.
Segher
