[9front] Weird >>= operator behaviour in 5c/5l

[9front] Weird >>= operator behaviour in 5c/5l

[Jonas Amoson]

Dear list,

It looks to me that the operation x >>= 0 doesn't generate the
correct result with 5c/5l (32-bit arm on Rpi 1b+). Example:

x = 2;
x >>= 0;

Gives x == 0 on arm, but on 386 it yields x == 2 as expected.
The non shorted form: x = x >> 0; Gives the correct result.
Is this a known issue, or I am missing something?

Looking at the assembly, the operation seems to be eliminated
if using the long form (giving the correct result) but using the
shorted form generates an instruction SRL $0, R1, R2, which
doesn't seem to work. Other shifts (e.g. x>>=1) do work (e.g.
generating SRL $1, R1, R2).

//Jonas Amoson

#include <u.h>
#include <libc.h>

void
main()
{
        unsigned int x = 2;
        print("%ud\n", x >>= 0);
        exits(nil);
}

Re: [9front] Weird >>= operator behaviour in 5c/5l

[Noam Preil]

It could just be architecture specific behavior.

Re: [9front] Weird >>= operator behaviour in 5c/5l

[ori]

Quoth Noam Preil <noam@pixelhero.dev>:
> It could just be architecture specific behavior.
> 

It's a bug. Two, actually.

1) a shift of zero should be optimized out by
   the compiler; cinap has a change for this in
   the works already. This fix should hide the
   second bug.

2) a shift by a literal zero is miscompiled by
   the assembler into a shift by 32, because
   of a missed special case in the instruction.
   encoding. I'm working on fixing the assembler.

Re: [9front] Weird >>= operator behaviour in 5c/5l

[Jonas Amoson]

Left shifting by 32 instead of 0 does explain my
result, and seam indeed to be the real bug.

Regarding optimising out shifts where the number
of steps is zero: it already does that when using the
pattern x = x >> 0; but it doesn't for x >>= 0;

Thanks,
Jonas

On Mon, 18 Jan 2021 at 07:50, <ori@eigenstate.org> wrote:
>
> Quoth Noam Preil <noam@pixelhero.dev>:
> > It could just be architecture specific behavior.
> >
>
> It's a bug. Two, actually.
>
> 1) a shift of zero should be optimized out by
>    the compiler; cinap has a change for this in
>    the works already. This fix should hide the
>    second bug.
>
> 2) a shift by a literal zero is miscompiled by
>    the assembler into a shift by 32, because
>    of a missed special case in the instruction.
>    encoding. I'm working on fixing the assembler.

Re: [9front] Weird >>= operator behaviour in 5c/5l

[ori]

Quoth Jonas Amoson <jonas.amoson@gmail.com>:
> Left shifting by 32 instead of 0 does explain my
> result, and seam indeed to be the real bug.
> 
> Regarding optimising out shifts where the number
> of steps is zero: it already does that when using the
> pattern x = x >> 0; but it doesn't for x >>= 0;
> 
> Thanks,
> Jonas
> 
> On Mon, 18 Jan 2021 at 07:50, <ori@eigenstate.org> wrote:
> >
> > Quoth Noam Preil <noam@pixelhero.dev>:
> > > It could just be architecture specific behavior.
> > >
> >
> > It's a bug. Two, actually.
> >
> > 1) a shift of zero should be optimized out by
> >    the compiler; cinap has a change for this in
> >    the works already. This fix should hide the
> >    second bug.
> >
> > 2) a shift by a literal zero is miscompiled by
> >    the assembler into a shift by 32, because
> >    of a missed special case in the instruction.
> >    encoding. I'm working on fixing the assembler.

I think this corrects the linker.

diff -r 01125acb5565 sys/src/cmd/5l/asm.c
--- a/sys/src/cmd/5l/asm.c	Tue Jan 19 19:56:38 2021 -0800
+++ b/sys/src/cmd/5l/asm.c	Tue Jan 19 21:56:58 2021 -0800
@@ -785,7 +785,7 @@
 
 	case 8:		/* sll $c,[R],R -> mov (R<<$c),R */
 		aclass(&p->from);
-		o1 = oprrr(p->as, p->scond);
+		o1 = oprrr((instoffset == 0) ? ASLL : p->as, p->scond);
 		r = p->reg;
 		if(r == NREG)
 			r = p->to.reg;

The PDF version of the arm architecture manual
I found implied that this is the correct way
to do it, and the official arm website is so
bad that I can't figure out if it contradicts
the PDF from some academic's website.

Only lightly tested, not sure if I have any
edge cases.

Re: [9front] Weird >>= operator behaviour in 5c/5l

[ori]

Quoth ori@eigenstate.org:
> Quoth Jonas Amoson <jonas.amoson@gmail.com>:
> > Left shifting by 32 instead of 0 does explain my
> > result, and seam indeed to be the real bug.
> > 
> > Regarding optimising out shifts where the number
> > of steps is zero: it already does that when using the
> > pattern x = x >> 0; but it doesn't for x >>= 0;
> > 
> > Thanks,
> > Jonas
> > 
> > On Mon, 18 Jan 2021 at 07:50, <ori@eigenstate.org> wrote:
> > >
> > > Quoth Noam Preil <noam@pixelhero.dev>:
> > > > It could just be architecture specific behavior.
> > > >
> > >
> > > It's a bug. Two, actually.
> > >
> > > 1) a shift of zero should be optimized out by
> > >    the compiler; cinap has a change for this in
> > >    the works already. This fix should hide the
> > >    second bug.
> > >
> > > 2) a shift by a literal zero is miscompiled by
> > >    the assembler into a shift by 32, because
> > >    of a missed special case in the instruction.
> > >    encoding. I'm working on fixing the assembler.
> 
> I think this corrects the linker.
> 
> diff -r 01125acb5565 sys/src/cmd/5l/asm.c
> --- a/sys/src/cmd/5l/asm.c	Tue Jan 19 19:56:38 2021 -0800
> +++ b/sys/src/cmd/5l/asm.c	Tue Jan 19 21:56:58 2021 -0800
> @@ -785,7 +785,7 @@
>  
>  	case 8:		/* sll $c,[R],R -> mov (R<<$c),R */
>  		aclass(&p->from);
> -		o1 = oprrr(p->as, p->scond);
> +		o1 = oprrr((instoffset == 0) ? ASLL : p->as, p->scond);
>  		r = p->reg;
>  		if(r == NREG)
>  			r = p->to.reg;
> 
> The PDF version of the arm architecture manual
> I found implied that this is the correct way
> to do it, and the official arm website is so
> bad that I can't figure out if it contradicts
> the PDF from some academic's website.
> 
> Only lightly tested, not sure if I have any
> edge cases.
> 
I like this version more.

diff -r 01125acb5565 sys/src/cmd/5l/asm.c
--- a/sys/src/cmd/5l/asm.c	Tue Jan 19 19:56:38 2021 -0800
+++ b/sys/src/cmd/5l/asm.c	Tue Jan 19 22:12:05 2021 -0800
@@ -789,6 +789,12 @@
 		r = p->reg;
 		if(r == NREG)
 			r = p->to.reg;
+		/*
+		 * R>>32 is encoded as R>>0, so flip to the
+		 * equivalent R<<0.
+		 */
+		if(instoffset == 0)
+			o1 &= ~(1<<5);
 		o1 |= r;
 		o1 |= (instoffset&31) << 7;
 		o1 |= p->to.reg << 12;

Re: [9front] Weird >>= operator behaviour in 5c/5l

[Jonas Amoson]

Great!
I have not tested your patch extensively yet, but Netsurf now runs
without graphical glitches on 32-bit arm on my old Rpi terminal.
Jonas

On Wed, 20 Jan 2021 at 08:39, <ori@eigenstate.org> wrote:
>
> Quoth ori@eigenstate.org:
> > Quoth Jonas Amoson <jonas.amoson@gmail.com>:
> > > Left shifting by 32 instead of 0 does explain my
> > > result, and seam indeed to be the real bug.
> > >
> > > Regarding optimising out shifts where the number
> > > of steps is zero: it already does that when using the
> > > pattern x = x >> 0; but it doesn't for x >>= 0;
> > >
> > > Thanks,
> > > Jonas
> > >
> > > On Mon, 18 Jan 2021 at 07:50, <ori@eigenstate.org> wrote:
> > > >
> > > > Quoth Noam Preil <noam@pixelhero.dev>:
> > > > > It could just be architecture specific behavior.
> > > > >
> > > >
> > > > It's a bug. Two, actually.
> > > >
> > > > 1) a shift of zero should be optimized out by
> > > >    the compiler; cinap has a change for this in
> > > >    the works already. This fix should hide the
> > > >    second bug.
> > > >
> > > > 2) a shift by a literal zero is miscompiled by
> > > >    the assembler into a shift by 32, because
> > > >    of a missed special case in the instruction.
> > > >    encoding. I'm working on fixing the assembler.
> >
> > I think this corrects the linker.
> >
> > diff -r 01125acb5565 sys/src/cmd/5l/asm.c
> > --- a/sys/src/cmd/5l/asm.c    Tue Jan 19 19:56:38 2021 -0800
> > +++ b/sys/src/cmd/5l/asm.c    Tue Jan 19 21:56:58 2021 -0800
> > @@ -785,7 +785,7 @@
> >
> >       case 8:         /* sll $c,[R],R -> mov (R<<$c),R */
> >               aclass(&p->from);
> > -             o1 = oprrr(p->as, p->scond);
> > +             o1 = oprrr((instoffset == 0) ? ASLL : p->as, p->scond);
> >               r = p->reg;
> >               if(r == NREG)
> >                       r = p->to.reg;
> >
> > The PDF version of the arm architecture manual
> > I found implied that this is the correct way
> > to do it, and the official arm website is so
> > bad that I can't figure out if it contradicts
> > the PDF from some academic's website.
> >
> > Only lightly tested, not sure if I have any
> > edge cases.
> >
> I like this version more.
>
> diff -r 01125acb5565 sys/src/cmd/5l/asm.c
> --- a/sys/src/cmd/5l/asm.c      Tue Jan 19 19:56:38 2021 -0800
> +++ b/sys/src/cmd/5l/asm.c      Tue Jan 19 22:12:05 2021 -0800
> @@ -789,6 +789,12 @@
>                 r = p->reg;
>                 if(r == NREG)
>                         r = p->to.reg;
> +               /*
> +                * R>>32 is encoded as R>>0, so flip to the
> +                * equivalent R<<0.
> +                */
> +               if(instoffset == 0)
> +                       o1 &= ~(1<<5);
>                 o1 |= r;
>                 o1 |= (instoffset&31) << 7;
>                 o1 |= p->to.reg << 12;
>

Re: [9front] Weird >>= operator behaviour in 5c/5l

[Anthony Martin]

ori@eigenstate.org once said:
> > Only lightly tested, not sure if I have any
> > edge cases.
> >
> I like this version more.
>
> diff -r 01125acb5565 sys/src/cmd/5l/asm.c
> --- a/sys/src/cmd/5l/asm.c	Tue Jan 19 19:56:38 2021 -0800
> +++ b/sys/src/cmd/5l/asm.c	Tue Jan 19 22:12:05 2021 -0800
> @@ -789,6 +789,12 @@
>  		r = p->reg;
>  		if(r == NREG)
>  			r = p->to.reg;
> +		/*
> +		 * R>>32 is encoded as R>>0, so flip to the
> +		 * equivalent R<<0.
> +		 */
> +		if(instoffset == 0)
> +			o1 &= ~(1<<5);
>  		o1 |= r;
>  		o1 |= (instoffset&31) << 7;
>  		o1 |= p->to.reg << 12;
>

This will affect any assembler source that uses

	ROR $0, R1, R2

to get at the special RRX arm instruction alias
for a rotate right by one bit with extension.

That instruction would become

	SRA $0, R1, R2

for a arithmetic shift right by 32 bits. It's
probably not used anywhere but I wanted to make
sure you knew about it.

For reference, here's my understanding of the
possible shift values on arm:

	a	i	stype	imm5
	SLL	LSL	00	0..31
	SRL	LSR	01	1..32 mod 32
	SRA	ASR	10	1..32 mod 32
	ROR	ROR	11	1..31
	ROR	RRX	11	0

Hope that helps.

Cheers,
  Anthony

Re: [9front] Weird >>= operator behaviour in 5c/5l

[ori]

> This will affect any assembler source that uses
> 
> 	ROR $0, R1, R2
> 
> to get at the special RRX arm instruction alias
> for a rotate right by one bit with extension.
> 
> That instruction would become
> 
> 	SRA $0, R1, R2
> 
> for a arithmetic shift right by 32 bits. It's
> probably not used anywhere but I wanted to make
> sure you knew about it.
> 
> For reference, here's my understanding of the
> possible shift values on arm:
> 
> 	a	i	stype	imm5
> 	SLL	LSL	00	0..31
> 	SRL	LSR	01	1..32 mod 32
> 	SRA	ASR	10	1..32 mod 32
> 	ROR	ROR	11	1..31
> 	ROR	RRX	11	0
> 
> Hope that helps.
> 
> Cheers,
>   Anthony
> 

Yeah -- I was thinking that a 0-byte ROR would be
a no-op, and it wouldn't matter -- but I think I
made a mistake. It seems like I should just make
check explicitly for the opcodes, and substitute
in oprrr().

diff -r 01125acb5565 sys/src/cmd/5l/asm.c
--- a/sys/src/cmd/5l/asm.c	Tue Jan 19 19:56:38 2021 -0800
+++ b/sys/src/cmd/5l/asm.c	Wed Jan 20 15:38:43 2021 -0800
@@ -785,7 +785,10 @@
 
 	case 8:		/* sll $c,[R],R -> mov (R<<$c),R */
 		aclass(&p->from);
-		o1 = oprrr(p->as, p->scond);
+		if((p->as == ASRL || p->as == ASRA) && instoffset == 0)
+			o1 = oprrr(ASLL, p->scond);
+		else
+			o1 = oprrr(p->as, p->scond);
 		r = p->reg;
 		if(r == NREG)
 			r = p->to.reg;