Re: [PATCH v2] Add ARM backend.

[Jan Nieuwenhuizen]

Danny Milosavljevic writes:

Hello Danny!

> On Sat,  6 Jun 2020 20:01:34 +0200
> Danny Milosavljevic <dannym@scratchpost.org> wrote:

(keeping all remarks inline without @Janneke too)

>> +++ b/lib/arm-mes/arm.M1
>> @@ -0,0 +1,401 @@
>> +### GNU Mes --- Maxwell Equations of Software
>> +### Copyright © 2017,2018 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
>> +### Copyright © 2019 Danny Milosavljevic <dannym@scratchpost.org>
>
> 2020
>
>> +DEFINE R14 E
>> +DEFINE PC F
>
> Add R9, although that whole thing is unused.
>
>> +DEFINE mov____$i8,%r1 10a0e3 # mov     r1, #66
>
> I should remove that useless comment.
>
>> +# fp -> r0
>> +DEFINE mov____%r11,%r0 0b00a0e1
>
> Is that used?
>
>> +DEFINE mov____%r0,0x32 04909fe5000089e5000000ea
>
> r1 and r2 can't happen?
>
>> +DEFINE mov____%r2,0x32 04909fe5002089e5000000ea
>
>> +DEFINE add____$i32,(%r1) 
>> 05002de9000091e50c209fe5020080e0000081e50500bde8000000ea
>
> r0 and r2 can't happen?

Only emitted by

;; FIXME: Implement M1 part.
(define (armv4:r-mem-add info v)
  (let ((r (get-r info)))
    `(,(if (< (abs v) #x80)
           `(,(string-append "add____$i32,(%" r ")") (#:immediate ,v))
           `(,(string-append "add____$i32,(%" r ")") (#:immediate ,v))))))

so...expr-add in compile.scm ... TBH, I don't know...This needs
(regaining) more intimate understanding of the code...

>> +DEFINE mov____0x32(%ebp),%r1 08909fe50b9089e0001099e5000000ea
>> +DEFINE mov____0x32(%ebp),%r0 08909fe50b9089e0000099e5000000ea
>
> The lines the other way around for better review?

(yeah)

>> +DEFINE add____$i32,%r1 04909fe5091091e0000000ea
>> +DEFINE add____$i32,%r0 04909fe5090090e0000000ea
>> +DEFINE add____$i32,%r2 04909fe5092092e0000000ea
>
> Sort the lines for better review?

(yeah)

>> +        (display (string-append "\n\n<\n:" name "\n"))
>
> Add comment that "<" aligns?

(yeah, I'd like an uptodate "legend" for M1 magic anyway)

>> +(define (armv4:call-label info label n)
>> +  `(((#:branch-offset3 ,label) bl)
>> +    ; FIXME: Can n be negative?
>> +    ((#:immediate1 ,(* n 4)) "add____$i8,%esp")))
>
> @Janneke: Can n be negative?  Probably not, but just to make sure.

No, it's called from

module/mescc/compile.scm (expr->register)

        [..]                 
        ((fctn-call (p-expr (ident ,name)) (expr-list . ,expr-list))
             (let* ((info (append-text info (ast->comment o)))
                    [..]
                    (n (length expr-list))
                    [..]
                                (append-text info (wrap-as (as info 'call-label 
name n))))

and is the number of arguments.

>> +(define (armv4:r0*r1 info)
>> +  ;; FIXME: Signedness.
>> +  (let ((r0 (get-r0 info))
>> +        (r1 (get-r1 info)))
>> +    `((,(string-append "mul____%" r0 ",%" r1)))))
>
> Probably would need a helper procedure again in libmescc.a to do that nicely.
>
> @Janneke: How come this doesn't have SIGNED? like the other ones below?

I don't know; I don't understand what's going on.  When reading this
code:

module/mescc/compile.scm (expr->register)

        ((mod ,a ,b) ((binop->r info) a b 'r0%r1
                      (or (signed? (ast->type a info)) (signed? (ast->type b 
info)))))
        ((mul ,a ,b) ((binop->r info) a b 'r0*r1))

and remember the test-driven way in which it was developed, I can (only)
imagine that we had no test case that needed a signed? flag.

So, now it's tempting to return the question: can we write a failing
multiplication test that needs such a signed? flag to implement
signed/unsigned multiplication branches?

>> +;; FIXME: lsr??! Signed or unsigned r0?
>> +(define (armv4:r0>>r1 info)
>> +  (let ((r0 (get-r0 info))
>> +        (r1 (get-r1 info)))
>> +    `((,(string-append "lsr____%" r0 ",%" r0 ",%" r1)))))
>
> Is r0 supposed to be signed or not?
>
> I take it that r1 > 0, right?

Looking at the code:

        ((rshift ,a ,b) ((binop->r info) a b 'r0>>r1))

it seems that signedness is "overlooked", and both registers can be
anything.

Hovever, we can be "fairly sure" that well-behaved code does not do
that, and would use "<<" with a positive r1.

$ cat foo.c 
int
main ()
{
  return -3 >> -5;
}

Look, gcc warns about this

$ gcc foo.c 
foo.c: In function ‘main’:
foo.c:4:13: warning: right shift count is negative [-Wshift-count-negative]
   return -3 >> -5;
             ^~
10:29:37 janneke@dundal:~/src/mes/mes [env]
$ ./a.out o
[160]

You can always see for yourself; try this diff:

$ git diff
diff --git a/module/mescc/compile.scm b/module/mescc/compile.scm
index 579de2ceb..999248bd3 100644
--- a/module/mescc/compile.scm
+++ b/module/mescc/compile.scm
@@ -1208,7 +1208,7 @@
         ((bitwise-or ,a ,b) ((binop->r info) a b 'r0-or-r1))
         ((bitwise-xor ,a ,b) ((binop->r info) a b 'r0-xor-r1))
         ((lshift ,a ,b) ((binop->r info) a b 'r0<<r1))
-        ((rshift ,a ,b) ((binop->r info) a b 'r0>>r1))
+        ((rshift ,a ,b) ((binop->r info) (pk "a" a) (pk "b" b) 'r0>>r1))
         ((div ,a ,b)
          ((binop->r info) a b 'r0/r1
           (or (signed? (ast->type a info)) (signed? (ast->type b info)))))

$ ./pre-inst-env mescc -L mescc-lib foo.c  # Hmm, this -L should not be 
necessary when using pre-inst-env :-(

;;; ("a" (neg (p-expr (fixed "3"))))

;;; ("b" (neg (p-expr (fixed "5"))))
10:32:37 janneke@dundal:~/src/mes/mes [env]
$ ./a.out 
[31]10:32:38 janneke@dundal:~/src/mes/mes [env]

Hmmm!  (This on x86).  So yeah, our mileage may vary (bah!).  Great catch!

>> +;; FIXME: Signed or not?
>> +(define (armv4:r0/r1 info signed?)
>> +  (let ((r0 (get-r0 info))
>> +        (r1 (get-r1 info)))
>> +    ;; __mesabi_uldiv(a, b, remainderp)
>> +    (cons* `(,(string-append "push___0"))
>> +           `(,(string-append "push___%" r1))
>> +           `(,(string-append "push___%" r0))
>> +           (armv4:call-label #f "__mesabi_uldiv" (* 4 3)))))
>
> Probably better to call something like __aeabi_idiv if signed.
>
>> +(define (armv4:r0%r1 info signed?)
>> +  (let ((r0 (get-r0 info))
>> +        (r1 (get-r1 info)))
>> +    ;; __mesabi_uldiv(a, b, remainderp)
>> +    (append `(("push___%r0") ; slot for remainder
>> +              ("mov____%esp,%r0")
>> +              ("push___%r0") ; pointer to remainder
>> +              (,(string-append "push___%" r1))
>> +              (,(string-append "push___%" r0)))
>> +            (armv4:call-label #f "__mesabi_uldiv" (* 4 3))
>> +            `(("pop____%r0")))))
>
> Probably better to call something like __aeabi_uidivmod if signed.
>
>> +    ("double" . ,(make-type 'float 4 #f)) ; FIXME
>> +    ("long double" . ,(make-type 'float 4 #f)) ; FIXME
>
> Weird...
>
> Although the i386 backend does that too.
>
> The x86_64 backend has sizes 8 and 8, respectively.  That's weird, too.
>
> I guess that's in order to pretend that value of these types fit into a 
> register?

Yes.  I had no idea how to do this properly.  To get tcc compiled, mescc
only needs to be able to parse double, float, "long long".

It would be nice to really support these!

> gcc 10 on i686-linux has 8 and 12, respectively.

(yeah)

We're getting there; and finding some bugs/ugliness on the way, very
happy with that!

Greetings,
Janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com