make check error in native compiler resulting from commit 1cd188799f86bcb13ad76e82e3436b1b7e9f9e9f on 2021-12-30.

make check error in native compiler resulting from commit 1cd188799f86bcb13ad76e82e3436b1b7e9f9e9f on 2021-12-30.

[Alan Mackenzie]

Hello, Andrea.

In that patch, I made the following change so that a comparison
involving Qnil would not go through the (newly) expensive emit_EQ:

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 0a10505257..8581fe8066 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -1829,9 +1829,7 @@ comp-limplify-lap-inst
       (byte-listp auto)
       (byte-eq auto)
       (byte-memq auto)
-      (byte-not
-       (comp-emit-set-call (comp-call 'eq (comp-slot-n (comp-sp))
-                                      (make-comp-mvar :constant nil))))
+      (byte-not null)
       (byte-car auto)
       (byte-cdr auto)
       (byte-cons auto)

..  Since then, there has been a mismatch in
test/lisp/emacs-lisp/comp-tests.log, comp-tests-ret-type-spec-55, where
the returned ret-type has now become t, whereas previously it was (not
integer).

I don't really understand what ret-type is.  The mismatch between what
this ret-type "should" be, and what it now is doesn't seem to be causing
any problems.  Would you please say whether this is, in fact, true.
Would you also please advise on the best way to fix this bug in make
check.

Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: make check error in native compiler resulting from commit 1cd188799f86bcb13ad76e82e3436b1b7e9f9e9f on 2021-12-30.

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.
>
> In that patch, I made the following change so that a comparison
> involving Qnil would not go through the (newly) expensive emit_EQ:
>
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 0a10505257..8581fe8066 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -1829,9 +1829,7 @@ comp-limplify-lap-inst
>        (byte-listp auto)
>        (byte-eq auto)
>        (byte-memq auto)
> -      (byte-not
> -       (comp-emit-set-call (comp-call 'eq (comp-slot-n (comp-sp))
> -                                      (make-comp-mvar :constant nil))))
> +      (byte-not null)
>        (byte-car auto)
>        (byte-cdr auto)
>        (byte-cons auto)
>
> ..  Since then, there has been a mismatch in
> test/lisp/emacs-lisp/comp-tests.log, comp-tests-ret-type-spec-55, where
> the returned ret-type has now become t, whereas previously it was (not
> integer).
>
> I don't really understand what ret-type is.  The mismatch between what
> this ret-type "should" be, and what it now is doesn't seem to be causing
> any problems.  Would you please say whether this is, in fact, true.
> Would you also please advise on the best way to fix this bug in make
> check.

Hi Alan,

ret-type is the return type of the function that the forward propagation
engine tries to compute.

============

(defun foo (x)
  (unless (integerp x)
    x))
(native-compile #'foo)
(subr-type (symbol-function #'foo))
=>
(function (t) (not integer))

;; OR

(subr-type (native-compile '(λ (x)
                              (unless (integerp x)
                                x))))
=>
(function (t) (not integer))
==========

Those two are probably now returning just:

(function (t) t)

I guess the fix should be around `comp-add-cond-cstrs' or
`comp-add-cond-cstrs-simple'.  I'll try to have a look later or tomorrow
(unless you are so curious you like to take the task ;)

Thanks!

  Andrea

Re: make check error in native compiler resulting from commit 1cd188799f86bcb13ad76e82e3436b1b7e9f9e9f on 2021-12-30.

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.
>
> In that patch, I made the following change so that a comparison
> involving Qnil would not go through the (newly) expensive emit_EQ:
>
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 0a10505257..8581fe8066 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -1829,9 +1829,7 @@ comp-limplify-lap-inst
>        (byte-listp auto)
>        (byte-eq auto)
>        (byte-memq auto)
> -      (byte-not
> -       (comp-emit-set-call (comp-call 'eq (comp-slot-n (comp-sp))
> -                                      (make-comp-mvar :constant nil))))
> +      (byte-not null)
>        (byte-car auto)
>        (byte-cdr auto)
>        (byte-cons auto)
>
> ..  Since then, there has been a mismatch in
> test/lisp/emacs-lisp/comp-tests.log, comp-tests-ret-type-spec-55, where
> the returned ret-type has now become t, whereas previously it was (not
> integer).
>
> I don't really understand what ret-type is.  The mismatch between what
> this ret-type "should" be, and what it now is doesn't seem to be causing
> any problems.  Would you please say whether this is, in fact, true.
> Would you also please advise on the best way to fix this bug in make
> check.
>
> Thanks!

Hi Alan,

while I'm fixing this I realize that we could make some substantial
improvement in performance for native code on the new setup.

Is good that with your current modifications we emit a call to `null' in
place of the complex `eq', but we really need to inline `null' as well.

This implies having an 'emit_null' function and to register it using
'register_emitter'.

WDYT?

  Andrea

Re: make check error in native compiler resulting from commit 1cd188799f86bcb13ad76e82e3436b1b7e9f9e9f on 2021-12-30.

[Andrea Corallo]

Andrea Corallo <akrl@sdf.org> writes:

> Alan Mackenzie <acm@muc.de> writes:
>
>> Hello, Andrea.
>>
>> In that patch, I made the following change so that a comparison
>> involving Qnil would not go through the (newly) expensive emit_EQ:
>>
>> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
>> index 0a10505257..8581fe8066 100644
>> --- a/lisp/emacs-lisp/comp.el
>> +++ b/lisp/emacs-lisp/comp.el
>> @@ -1829,9 +1829,7 @@ comp-limplify-lap-inst
>>        (byte-listp auto)
>>        (byte-eq auto)
>>        (byte-memq auto)
>> -      (byte-not
>> -       (comp-emit-set-call (comp-call 'eq (comp-slot-n (comp-sp))
>> -                                      (make-comp-mvar :constant nil))))
>> +      (byte-not null)
>>        (byte-car auto)
>>        (byte-cdr auto)
>>        (byte-cons auto)

Yeah, I think reverting this hunk and keeping the `eq' explicit in
LIMPLE is the right thing to do, it solves this specific issue and we
get back null and not inlined for free.

The explicit `eq' gets already nicely handled by the logic you have
added in comp.c:2244 so we emit libgccjit IR using emit_BASE_EQ in this
case.

I've pushed dcf30f14f9 to do that as it works here.

Thanks

  Andrea

Re: make check error in native compiler resulting from commit 1cd188799f86bcb13ad76e82e3436b1b7e9f9e9f on 2021-12-30.

[Alan Mackenzie]

Hello, Andrea.

On Fri, Feb 04, 2022 at 14:58:47 +0000, Andrea Corallo wrote:
> Andrea Corallo <akrl@sdf.org> writes:

> > Alan Mackenzie <acm@muc.de> writes:
> >> In that patch, I made the following change so that a comparison
> >> involving Qnil would not go through the (newly) expensive emit_EQ:

> >> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> >> index 0a10505257..8581fe8066 100644
> >> --- a/lisp/emacs-lisp/comp.el
> >> +++ b/lisp/emacs-lisp/comp.el
> >> @@ -1829,9 +1829,7 @@ comp-limplify-lap-inst
> >>        (byte-listp auto)
> >>        (byte-eq auto)
> >>        (byte-memq auto)
> >> -      (byte-not
> >> -       (comp-emit-set-call (comp-call 'eq (comp-slot-n (comp-sp))
> >> -                                      (make-comp-mvar :constant nil))))
> >> +      (byte-not null)
> >>        (byte-car auto)
> >>        (byte-cdr auto)
> >>        (byte-cons auto)

> Yeah, I think reverting this hunk and keeping the `eq' explicit in
> LIMPLE is the right thing to do, it solves this specific issue and we
> get back null and not inlined for free.

> The explicit `eq' gets already nicely handled by the logic you have
> added in comp.c:2244 so we emit libgccjit IR using emit_BASE_EQ in this
> case.

> I've pushed dcf30f14f9 to do that as it works here.

OK, thank you!

> Thanks

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).