* [9front] Weird >>= operator behaviour in 5c/5l
@ 2021-01-17 21:36 Jonas Amoson
2021-01-18 4:05 ` Noam Preil
0 siblings, 1 reply; 9+ messages in thread
From: Jonas Amoson @ 2021-01-17 21:36 UTC (permalink / raw)
To: 9front
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);
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-17 21:36 [9front] Weird >>= operator behaviour in 5c/5l Jonas Amoson
@ 2021-01-18 4:05 ` Noam Preil
2021-01-18 5:20 ` ori
0 siblings, 1 reply; 9+ messages in thread
From: Noam Preil @ 2021-01-18 4:05 UTC (permalink / raw)
To: Jonas Amoson
It could just be architecture specific behavior.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-18 4:05 ` Noam Preil
@ 2021-01-18 5:20 ` ori
2021-01-18 16:11 ` Jonas Amoson
0 siblings, 1 reply; 9+ messages in thread
From: ori @ 2021-01-18 5:20 UTC (permalink / raw)
To: 9front
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-18 5:20 ` ori
@ 2021-01-18 16:11 ` Jonas Amoson
2021-01-20 5:59 ` ori
0 siblings, 1 reply; 9+ messages in thread
From: Jonas Amoson @ 2021-01-18 16:11 UTC (permalink / raw)
To: 9front
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-18 16:11 ` Jonas Amoson
@ 2021-01-20 5:59 ` ori
2021-01-20 6:12 ` ori
0 siblings, 1 reply; 9+ messages in thread
From: ori @ 2021-01-20 5:59 UTC (permalink / raw)
To: 9front
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-20 5:59 ` ori
@ 2021-01-20 6:12 ` ori
2021-01-20 12:06 ` Jonas Amoson
2021-01-20 22:11 ` Anthony Martin
0 siblings, 2 replies; 9+ messages in thread
From: ori @ 2021-01-20 6:12 UTC (permalink / raw)
To: 9front
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-20 6:12 ` ori
@ 2021-01-20 12:06 ` Jonas Amoson
2021-01-20 22:11 ` Anthony Martin
1 sibling, 0 replies; 9+ messages in thread
From: Jonas Amoson @ 2021-01-20 12:06 UTC (permalink / raw)
To: 9front
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;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-20 6:12 ` ori
2021-01-20 12:06 ` Jonas Amoson
@ 2021-01-20 22:11 ` Anthony Martin
2021-01-20 23:39 ` ori
1 sibling, 1 reply; 9+ messages in thread
From: Anthony Martin @ 2021-01-20 22:11 UTC (permalink / raw)
To: 9front
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9front] Weird >>= operator behaviour in 5c/5l
2021-01-20 22:11 ` Anthony Martin
@ 2021-01-20 23:39 ` ori
0 siblings, 0 replies; 9+ messages in thread
From: ori @ 2021-01-20 23:39 UTC (permalink / raw)
To: 9front
> 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;
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-21 0:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-17 21:36 [9front] Weird >>= operator behaviour in 5c/5l Jonas Amoson
2021-01-18 4:05 ` Noam Preil
2021-01-18 5:20 ` ori
2021-01-18 16:11 ` Jonas Amoson
2021-01-20 5:59 ` ori
2021-01-20 6:12 ` ori
2021-01-20 12:06 ` Jonas Amoson
2021-01-20 22:11 ` Anthony Martin
2021-01-20 23:39 ` ori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).