| Message ID | 02bf51a9-532d-8fdc-a2b2-083cee308eb7@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | One more patch for PR89676 | expand |
> On Mar 26, 2019, at 12:22 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: > > Jeff Law recently found that my latest patch break some existing code compilation (the code is big to make test out of it). > > Here is the patch to fix it. The patch was successfully bootstrapped on x86-64. The patch actually results in less new transformations the previous patch introduced. So it should be safe. > > Committed as rev. 269924. Hi Vladimir, FWIW, this fixed linux kernel builds on AArch64 and ARM, which your first patch caused. The failure was: === slub.s: Assembler messages: slub.s:26: Error: reg pair must start from even reg at operand 1 -- `casp x1,x0,x3,x5,[x6]' === from a reduced testcase: === void *a; long b, c; void d(void) { typeof(0) e=0; asm(" casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n" : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a) : [new1] "r"(d), [new2] "r"(e)); } === Is this the same bug that Jeff reported? Thanks, -- Maxim Kuvyrkov www.linaro.org
On 3/26/19 4:25 AM, Maxim Kuvyrkov wrote: >> On Mar 26, 2019, at 12:22 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: >> >> Jeff Law recently found that my latest patch break some existing code compilation (the code is big to make test out of it). >> >> Here is the patch to fix it. The patch was successfully bootstrapped on x86-64. The patch actually results in less new transformations the previous patch introduced. So it should be safe. >> >> Committed as rev. 269924. > Hi Vladimir, > > FWIW, this fixed linux kernel builds on AArch64 and ARM, which your first patch caused. > > The failure was: > === > slub.s: Assembler messages: > slub.s:26: Error: reg pair must start from even reg at operand 1 -- `casp x1,x0,x3,x5,[x6]' > === > > from a reduced testcase: > === > void *a; > long b, c; > void d(void) { > typeof(0) e=0; > asm(" casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n" > : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a) > : [new1] "r"(d), [new2] "r"(e)); > } > === > > Is this the same bug that Jeff reported? > Yes, it looks similar (reg pair and casp). I did know it was a kernel. I only had a preprocessed file slub.i. The reduced case is actually wrong. The constraints +&r does not guarantee that b and c will be in the subsequent regs. In the original test case b and c were also local *register* variables (x0 and x1). Generally speaking even this does no guarantee (according to the documentation) that the original variable values will be in subsequent regs for casp. But as people assume such (undocumented) semantics, we should maintain this.
> On Mar 26, 2019, at 3:20 PM, Vladimir Makarov <vmakarov@redhat.com> wrote: > > On 3/26/19 4:25 AM, Maxim Kuvyrkov wrote: >>> On Mar 26, 2019, at 12:22 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: >>> >>> Jeff Law recently found that my latest patch break some existing code compilation (the code is big to make test out of it). >>> >>> Here is the patch to fix it. The patch was successfully bootstrapped on x86-64. The patch actually results in less new transformations the previous patch introduced. So it should be safe. >>> >>> Committed as rev. 269924. >> Hi Vladimir, >> >> FWIW, this fixed linux kernel builds on AArch64 and ARM, which your first patch caused. >> >> The failure was: >> === >> slub.s: Assembler messages: >> slub.s:26: Error: reg pair must start from even reg at operand 1 -- `casp x1,x0,x3,x5,[x6]' >> === >> >> from a reduced testcase: >> === >> void *a; >> long b, c; >> void d(void) { >> typeof(0) e=0; >> asm(" casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n" >> : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a) >> : [new1] "r"(d), [new2] "r"(e)); >> } >> === >> >> Is this the same bug that Jeff reported? >> > Yes, it looks similar (reg pair and casp). I did know it was a kernel. I only had a preprocessed file slub.i. > > The reduced case is actually wrong. The constraints +&r does not guarantee that b and c will be in the subsequent regs. Right, this was reduction artifact. > In the original test case b and c were also local *register* variables (x0 and x1). Generally speaking even this does no guarantee (according to the documentation) that the original variable values will be in subsequent regs for casp. I don't follow. Do you mean that in the below testcase it's not guaranteed that casp will get its first two arguments in x0 and x1? (If so, why?) === void *a; long b, c; void d(void) { typeof(0) e=0; register long x0 asm ("x0") = b; register long x1 asm ("x1") = c; asm(" casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n" : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a) : [new1] "r"(d), [new2] "r"(e)); } === IIRC, there is plenty of syscall code in glibc that relies on asms getting variables in "right" registers. > But as people assume such (undocumented) semantics, we should maintain this. Regards, -- Maxim Kuvyrkov www.linaro.org
On 3/26/19 8:35 AM, Maxim Kuvyrkov wrote: > > I don't follow. Do you mean that in the below testcase it's not guaranteed that casp will get its first two arguments in x0 and x1? (If so, why?) Sorry for not to be clear. With my first patch only, it was not guaranteed for some complicated code cases. With the additional patch it is guaranteed as it was before. > === > void *a; > long b, c; > void d(void) { > typeof(0) e=0; > register long x0 asm ("x0") = b; > register long x1 asm ("x1") = c; > asm(" casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n" > : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a) > : [new1] "r"(d), [new2] "r"(e)); > } > > >
Index: ChangeLog =================================================================== --- ChangeLog (revision 269923) +++ ChangeLog (working copy) @@ -1,3 +1,10 @@ +2019-03-25 Vladimir Makarov <vmakarov@redhat.com> + + PR rtl-optimization/89676 + * lra-constraints.c (curr_insn_transform): Do match reload for + early clobbers when the match was successful only for different + registers. + 2019-03-25 Martin Sebor <msebor@redhat.com> * doc/extend.texi (Common Type Attributes): Document vector_size. Index: lra-constraints.c =================================================================== --- lra-constraints.c (revision 269878) +++ lra-constraints.c (working copy) @@ -4259,15 +4259,27 @@ curr_insn_transform (bool check_only_p) else if (goal_alt_matched[i][0] != -1 && curr_static_id->operand[i].type == OP_OUT && (curr_static_id->operand_alternative - [goal_alt_number * n_operands + i].earlyclobber)) + [goal_alt_number * n_operands + i].earlyclobber) + && REG_P (op)) { - /* Generate reloads for output and matched inputs. This - is the easiest way to avoid creation of non-existing - conflicts in lra-lives.c. */ - match_reload (i, goal_alt_matched[i], outputs, goal_alt[i], &before, - &after, TRUE); - outputs[n_outputs++] = i; - outputs[n_outputs] = -1; + for (j = 0; goal_alt_matched[i][j] != -1; j++) + { + rtx op2 = *curr_id->operand_loc[goal_alt_matched[i][j]]; + + if (REG_P (op2) && REGNO (op) != REGNO (op2)) + break; + } + if (goal_alt_matched[i][j] != -1) + { + /* Generate reloads for different output and matched + input registers. This is the easiest way to avoid + creation of non-existing register conflicts in + lra-lives.c. */ + match_reload (i, goal_alt_matched[i], outputs, goal_alt[i], &before, + &after, TRUE); + outputs[n_outputs++] = i; + outputs[n_outputs] = -1; + } continue; } else