| Message ID | 56B387BF.6050902@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/04/2016 12:17 PM, Bernd Schmidt wrote: > In this PR, we have, at an intermediate stage during LRA (before > create_cands): > > (insn 420 (set (reg:HI 276 [orig:132 g.2_118 ] [132]) > (reg:HI 132 [ g.2_118 ])) 88 {*movhi_internal} > (nil)) > [....] > (insn 436 (set (reg/v:HI 290 [orig:87 g ] [87]) > (reg/v:HI 87 [ g ])) > > (insn 14 (set (reg/v:HI 88 [ l ]) > (reg/v:HI 290 [orig:87 g ] [87])) > > (insn 15 (set (reg/v:HI 87 [ g ]) > (reg:HI 276 [orig:132 g.2_118 ] [132])) > > During create_cands, we recognize 420 as a potential candidate, and > when we reach insn 15, we select it as insn2, and create a new > candidate for register 87. Later, in do_remat, that candidate is > chosen when processing insn 436: we choose to use register 132 instead > of register 87: > > 436: r290:HI=r87:HI > REG_DEAD r87:HI > Inserting rematerialization insn before: > 441: r290:HI=r132:HI > > This is kind of bad, because the equivalencing insn (#15) comes later. > > After a few false starts, I came up with the patch below, which keeps > track of not just the candidate insn, but also an activation insn, and > chooses candidates only if they are both available and active. Besides > passing a new arg to create_cand, the changes in create_cands are > mostly cosmetic to make the function less confusing. This was > bootstrapped and tested on x86_64-linux. Ok? > The patch looks ok for me. Thanks for working on the PR, Bernd. > > As an aside - I have some doubts about whether the liveness > calculations in that file are completely safe. I see lots of direct > register number comparisons in functions like input_regno_present_p, > which is fine for pseudos, but if hard regs are involved I can't quite > convince myself that the code is safe in the presence of overlaps. For > now I've left it alone, but for gcc-7 this code should be reworked IMO. > I can not see that liveness calculation is wrong (hard registers modes are not present in lra insn regs, e.g. if insn refers double-word hard reg because of its mode, the two hard regs are present in lra insn regs). But it would be nice if you look at this code when you have time and willingness.
On 02/04/2016 09:27 PM, Vladimir Makarov wrote: >> After a few false starts, I came up with the patch below, which keeps >> track of not just the candidate insn, but also an activation insn, and >> chooses candidates only if they are both available and active. Besides >> passing a new arg to create_cand, the changes in create_cands are >> mostly cosmetic to make the function less confusing. This was >> bootstrapped and tested on x86_64-linux. Ok? >> > The patch looks ok for me. Thanks for working on the PR, Bernd. I should get in the habit of asking "ok everywhere?" Can I put this on gcc-5-branch as well? Bernd
On 02/15/2016 02:13 PM, Bernd Schmidt wrote: > On 02/04/2016 09:27 PM, Vladimir Makarov wrote: >>> After a few false starts, I came up with the patch below, which keeps >>> track of not just the candidate insn, but also an activation insn, and >>> chooses candidates only if they are both available and active. Besides >>> passing a new arg to create_cand, the changes in create_cands are >>> mostly cosmetic to make the function less confusing. This was >>> bootstrapped and tested on x86_64-linux. Ok? >>> >> The patch looks ok for me. Thanks for working on the PR, Bernd. > > I should get in the habit of asking "ok everywhere?" Can I put this on > gcc-5-branch as well? ... and I managed to confuse myself about which LRA issue I had approval for on gcc-5-branch, and checked it in. Sorry. If I don't hear back until tomorrow I'll revert it. Bernd
On 02/16/2016 04:20 PM, Bernd Schmidt wrote: > > > On 02/15/2016 02:13 PM, Bernd Schmidt wrote: >> On 02/04/2016 09:27 PM, Vladimir Makarov wrote: >>>> After a few false starts, I came up with the patch below, which keeps >>>> track of not just the candidate insn, but also an activation insn, and >>>> chooses candidates only if they are both available and active. Besides >>>> passing a new arg to create_cand, the changes in create_cands are >>>> mostly cosmetic to make the function less confusing. This was >>>> bootstrapped and tested on x86_64-linux. Ok? >>>> >>> The patch looks ok for me. Thanks for working on the PR, Bernd. >> >> I should get in the habit of asking "ok everywhere?" Can I put this on >> gcc-5-branch as well? > > ... and I managed to confuse myself about which LRA issue I had > approval for on gcc-5-branch, and checked it in. Sorry. If I don't > hear back until tomorrow I'll revert it. > Sorry, Bernd. I should be more clear too. The patch is ok for any GCC version with lra-remat.c (it includes gcc-5 and coming gcc-6).
>> ... and I managed to confuse myself about which LRA issue I had >> approval for on gcc-5-branch, and checked it in. Sorry. If I don't >> hear back until tomorrow I'll revert it. >> > Sorry, Bernd. I should be more clear too. The patch is ok for any GCC > version with lra-remat.c (it includes gcc-5 and coming gcc-6). Thanks, Vlad. I'll leave it in then :) Bernd
PR rtl-optimization/68730 * gcc.dg/torture/pr68730.c: New test. diff --git a/gcc/testsuite/gcc.dg/torture/pr68730.c b/gcc/testsuite/gcc.dg/torture/pr68730.c new file mode 100644 index 0000000..3036b64 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr68730.c @@ -0,0 +1,68 @@ +/* { dg-do run } */ + +#include <stdarg.h> +volatile va_list v; +volatile int z = 0; +__attribute__((noinline,noclone)) int no_printf (const char *p, ...) +{ + va_list x; + va_start (x, p); + while (z) + z = va_arg (x, int); + return 0; +} + +int b, d, e; +unsigned long long c = 4100543410106915; + +void +fn1 () +{ + short f, g = 4 % c; + int h = c; + if (h) + { + int i = ~c; + if (~c) + i = 25662; + f = g = i; + h = c - g + ~-f; + c = ~(c * h - f); + } + f = g; + unsigned long long k = g || c; + short l = c ^ g ^ k; + if (g > 25662 || c == 74074520320 || !(g < 2)) + { + k = c; + l = g; + c = ~((k && c) + ~l); + f = ~(f * (c ^ k) | l); + if (c > k) + no_printf ("", f); + } + short m = -f; + unsigned long long n = c; + c = m * f | n % c; + if (n) + no_printf ("", f, g, l); + while (f < -31807) + ; + c = ~(n | c) | f; + if (n < c) + no_printf ("", (long long) f, h, l); + for (; d;) + for (; e;) + for (;;) + ; + c = h; + c = l % c; +} + +int +main () +{ + for (; b < 2; b++) + fn1 (); + return 0; +}
In this PR, we have, at an intermediate stage during LRA (before create_cands): (insn 420 (set (reg:HI 276 [orig:132 g.2_118 ] [132]) (reg:HI 132 [ g.2_118 ])) 88 {*movhi_internal} (nil)) [....] (insn 436 (set (reg/v:HI 290 [orig:87 g ] [87]) (reg/v:HI 87 [ g ])) (insn 14 (set (reg/v:HI 88 [ l ]) (reg/v:HI 290 [orig:87 g ] [87])) (insn 15 (set (reg/v:HI 87 [ g ]) (reg:HI 276 [orig:132 g.2_118 ] [132])) During create_cands, we recognize 420 as a potential candidate, and when we reach insn 15, we select it as insn2, and create a new candidate for register 87. Later, in do_remat, that candidate is chosen when processing insn 436: we choose to use register 132 instead of register 87: 436: r290:HI=r87:HI REG_DEAD r87:HI Inserting rematerialization insn before: 441: r290:HI=r132:HI This is kind of bad, because the equivalencing insn (#15) comes later. After a few false starts, I came up with the patch below, which keeps track of not just the candidate insn, but also an activation insn, and chooses candidates only if they are both available and active. Besides passing a new arg to create_cand, the changes in create_cands are mostly cosmetic to make the function less confusing. This was bootstrapped and tested on x86_64-linux. Ok? As an aside - I have some doubts about whether the liveness calculations in that file are completely safe. I see lots of direct register number comparisons in functions like input_regno_present_p, which is fine for pseudos, but if hard regs are involved I can't quite convince myself that the code is safe in the presence of overlaps. For now I've left it alone, but for gcc-7 this code should be reworked IMO. Bernd