On Tue, Jan 10, 2023 at 4:01 PM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> Hi Richard and Uros,
> I believe I've managed to reduce a minimal test case that exhibits the
> underlying
> problem with reload. The following snippet when compiled on x86-64 with
> -O2:
>
> void ext(int x);
> void foo(int x, int y) { ext(y - x); }
>
> produces the following 5 instructions prior to reload:
> insn 13: r86:SI=di:SI // REG_DEAD di:SI
> insn 14: r87:SI=si:SI // REG_READ si:SI
> insn 7: {r85:SI=r87:SI-r86:SI;clobber flags:CC;} // REG_DEAD r86:SI,
> r87:SI
> insn 8: di:SI=r85:SI // REG_READ r85:SI
> insn 9: call [`ext'] argc:0
>
> Hence there are three pseudos (allocnos) to be register allocated; r85, r86
> & r87.
>
> Currently, reload produces the following assignments/colouring using 3 hard
> regs.
> r85 in di
> r86 in ax
> r87 in si
>
> A better (optimal) register allocation requires only 2 hard regs.
> r85 in di
> r86 in si
> r87 in di
>
> Fortunately, this over-allocation is cleaned up later (during
> cprop_hardreg), but
> as pointed out by Uros, there's little benefit in reducing register pressure
> this
> late (after peephole2).
>
> As far as I understand it, Richard's patch to handle fully-tied destinations
> looks
> very reasonable (and is impressively tested/benchmarked):
> https://gcc.gnu.org/pipermail/gcc-patches/2019-September/530743.html
> but in the prototypical 0:"=r", 1:"0", 2:"r" constraint case, as used in the
> problematic subsi3_1 pattern (of insn 7), I'm trying to figure out why r85
> and r87 don't get allocated to the same register [given the local spilling
> of non-eliminable hard regs in insn 7, temporarily introducing a new pseudo
> r89].
>
> In closing, reload is a complex piece of code that's shared between a large
> number of backends; if Richard's patch is a win "statistically", then it's
> not unreasonable to use a peephole2 to clean-up/catch the corner cases
> on class_likely_spilled_p targets [indeed many of the peephole2s in i386.md
> tidy up register allocation issues], and such a "specialized" fix is more
> suitable
> for stage 3, than a potentially disruptive tweak to reload. At worst, the
> peephole2 becomes dead if/when the problem is fixed upstream.
>
> Or put another way, if reload worked perfectly, i386.md wouldn't need
> many of the peephole2s that it currently has. Oh, for such an ideal world.
I have benchmarked the new peephole a bit and during the build of
linux kernel and during the whole gcc bootstrap, it didn't trigger
even once. It looks to me that the compiler produces the problematic
sequence only for specially crafted testcases, when argument setup is
involved. These testcases expose a minor annoyance with the reload
(which IMO should be fixed in the reload and not papered over with a
peephole).
Technically, the pattern is OK, but it really doesn't bring much to
the table. OTOH, the pattern is simple enough that it won't hurt if we
have another specialized pattern in the .md file. I'll leave the
decision to you.
Uros.
