Em sex., 9 de set. de 2022 às 13:53, Paul Cercueil
<paul@crapouillou.net> escreveu:
>
>
>
> Le ven., sept. 9 2022 at 12:30:33 -0300, Paulo César Pereira de
> Andrade <paulo.cesar.pereira.de.andrade@gmail.com> a écrit :
> > Em sex., 9 de set. de 2022 às 12:08, Paulo César Pereira de
Andrade
> > <paulo.cesar.pereira.de.andrade@gmail.com> escreveu:
> >>
> >> Em sex., 9 de set. de 2022 às 11:42, Paul Cercueil
> >> <paul@crapouillou.net> escreveu:
> >> >
> >> > Hi Paulo,
> >> >
> >> > [snip]
> >> >
> >> > >> The very last break happened as it was calling
jit_new_state()
> >> > >> again,
> >> > >> so I suppose the memory address that was being watched
was
> >> being
> >> > >> reused
> >> > >> and I stopped the debugging session there.
> >> > >
> >> > > Please just do call jit_print() after finishing
> >> split_branches, for
> >> > > example:
> >> > >
> >> > > Breakpoint 1, _split_branches (_jit=0x10111270) at
> >> lightning.c:2713
> >> > > 2713 for (node = _jitc->head; node; node = next) {
> >> > > (gdb) call _jit_print(_jit)
> >> > > L0: /* prolog */
> >> > > #name
> >> > > #note 395
> >> > > ...
> >> > >
> >> > > and let me know the output. The likely explanation for it
not
> >> being
> >> > > set,
> >> > > assuming lightning code is not modified, is that there is
some
> >> extra
> >> > > code changing the value of r26 after it is used as a
temporary.
> >> > > Something
> >> > > like in the range from "L2:" to the "stxi_i 0x3e7c r29
r26"
> >> below,
> >> > > there is
> >> > > some instruction that changes r26, and causes it to
assume it
> >> is dead
> >> > > in
> >> > > that range, and usable as a temporary.
> >> >
> >> > I'll do better, I'll give you an example that reproduces the
> >> issue :)
> >> > See the attached example.
> >> >
> >> > Your idea that something must be changing r26 in that slice
of
> >> code
> >> > pointed me in the right direction.
> >> > I only changed one line in the example: there is now a
> >> jit_movi(r26, 0)
> >> > line 58.
> >> >
> >> > I assume Lightning detects that r26 is already 0 anyway
(because
> >> of the
> >> > beqi just before) so it removes this instruction, causing
havoc
> >> at the
> >> > same time.
> >>
> >> Exactly, the instruction is being removed, and the optimizer
> >> getting
> >> confused.
> >> This should not happen as the optimizations done are
supposed to
> >> be the fool proof ones, but somehow this pattern triggered a
bug.
> >> I will try to understand it and have a patch later today.
> >
> > Just in case, this should work as a temporary workaround:
> >
> > """
> > diff --git a/lib/lightning.c b/lib/lightning.c
> > index d219e6d..c4f2e91 100644
> > --- a/lib/lightning.c
> > +++ b/lib/lightning.c
> > @@ -1702,12 +1702,14 @@ _jit_optimize(jit_state_t *_jit)
> > case jit_code_epilog:
> > _jitc->function = NULL;
> > break;
> > +#if 0
> > case jit_code_beqi:
> > redundant_store(node, 1);
> > break;
> > case jit_code_bnei:
> > redundant_store(node, 0);
> > break;
> > +#endif
> > default:
> > #if JIT_HASH_CONSTS
> > if (mask & jit_cc_a0_flt) {
> > """
>
> I simply commented the 'redunant_store()' calls as I didn't know
if it
> was OK for the jit_code_beqi/jit_code_bnei cases to fall in the
> "default" case.
It should go to the default case, because it might need to
allocate
a constant to be loaded, or if in a function body, need to know the
register is modified, what only matters if it is callee save; this
was
a second problem in this code path; if it did not remove the movi
in redundant_store() it should still go to the default case, or,
could
not detect a callee save register is modified (likely it was already
noted as modified, but maybe in some very special scenario it
could not notice that...).
> Looks like the workaround works, thanks for the suggestion.
The problem was that it basically did add an "implicit" note note
that
the register was dead in the range from L2 to the
"jit_movi(JIT_V1, 0)",
so, it was safe to use it as temporary, but when removing the
jit_movi
in another optimization pass, it broke the logic.
> > I will try to work on a patch, to have the live register
computation
> > work as if the movi was still done, or just remove and no
longer use
> > redundant_store, as this movi removal is causing the problem.
> >
> > """
> > case jit_code_movi:
> > if (regno == jit_regno(iter->u.w)) {
> > if (iter->flag || iter->v.w != word)
> > return;
> > del_node(prev, iter);
> > iter = prev;
> > }
> > break;
> > """
> >
> > The problem is that the instruction is removed after the
computation
> > of the initial state at the start of the block.