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

[Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Pip Cet <pipcet@gmail.com> writes:

> 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.

This new mvar *is* used, actually by an assume.  And the assume in
discussion is targetting a live variable that will be used by functional
code so I really see no scandal here.

>> 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.

Again no, any pass in the compiler must have the right to create new
temporaries for whichever purpose it wants, and indeed we can't expect
the byte-compiler to know in advance all passes in the native compiler
and their scopes and goals.  `add-cstrs' is just the first pass that now
happen to have this need, is not a special case of any sort.

Not giving the possibilities to passes to create variables would be
totally equivalent to prevent GIMPLE passes in GCC to create new local
variables while performing transformations, it would be ludicrous and
this is just something we *want* to be able to do.

  Andrea