bug#46670: 28.0.50; [feature/native-comp] possible miscompilation affecting lsp-mode

[Pip Cet]

On Mon, Feb 22, 2021 at 11:16 AM Andrea Corallo <akrl@sdf.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> >> Yes but in this case (and probably others) we *have* to emit this
> >> assumption.
> >
> > Why? Are you saying the compiler requires (assume ...) insns for
> > correctness? If it does, I'm afraid that's a serious issue.
>
> It requires that assume if we want to infer precisely here.  If we
> give-up emitting this assume it will just works perfectly but we'll miss
> to predict the return value as we should.

Emitting an assumption about a dead variable only makes sense if the
dead variable matches a live one. And in that case, we can just emit
the assumption about the live variable to begin with.

> > Again, that does seem very complicated, and if it improves
> > optimization we could probably improve it much more by modifying the
> > byte compiler to pop arguments in the caller rather than the callee.
>
> To me this sounds considerably more invasive, probably is because I'm
> much more used to work with the native compiler code that with the byte
> compiler one :)

That is a little more invasive, but it's where you're going if you
make the major change of allocating your own stack slots rather than
letting the byte compiler do it.

> I like the idea of the native compiler patch being less invasive as
> possible, this was the line I tried to follow and I think so far it
> paid.  I guess a number of readers would'd agree with this kind of
> approach to begin with.

I agree with it! That's why "leave slot allocation to the byte
compiler" is something I don't want you to throw away unnecessarily,
because it's a great simplifying assumption.

> I think I should be able to work it out as discussed in one two evenings
> and it might be useful for other cases in the future too, so it does not
> sound as a big deal to me.

It does to me, I must say. There's nothing special about comparisons:
you're turning a = a OP b two-operand code into c = a OP b
three-operand code. Your code won't be as good as genuine
three-operand code, and it won't be as simple as two-operand code.

> >> > In fact, all we need to do is tell comp-cond-cstrs-target-mvar to
> >> > return nil more often, isn't it?
> >>
> >> Nope, the target mvar identified is the correct one, we just have ATM no
> >> way to reference it reliably into the assume.
> >
> > We don't have to, let's just not emit an assume about a variable that
> > we just introduced and that's never read?
>
> We disagree :)

We can disagree about the "let's" part (and should, of course), but we
should agree about the "we don't have to" part, because that's a
matter of fact, not opinion.

> As the byte compiler does not care about propagating types and values it
> can just clobber the variable, here we aim for more so we need it to
> keep it live till the assume.

If we decide the variable needs to be kept live, the byte compiler
should keep it live, not us.