Re: [hatari-devel] DSP add AGU pipeline for parallel moves

[Laurent Sallafranque <laurent.sallafranque@xxxxxxx>]

> Thank you for fixing this bug.  

You're welcome.


> Btw. this did not just cause a warning, the code would have failed.

That's strange. I don't get any warning and I've done a lot of tests and everything worked well.
But no problem to fix things if needed.


> In the old code m_reg was uint16_t. 
That's right, I'll change this. This will allow to test with $ffff instead of -1 wich is closer to the motorola doc.


> It probably makes sense to check the new code for other signed/unsigned data type errors. These errors are very hard to debug if discovered later. 
I will also.


> And I suggest to remove the commented lines.

I don't agree with this one. Movem is said to be a parallel mode instruction in the Motorola doc.
But my tests didn't show it.
But maybe I didn't understand the doc correctly.
I would prefer to add a comment like :
According to Motorola doc, MOVEM is said to be a parralel move instruction.
But tests seems to contradict this.
Next line should stay commented for now.


This would avoid to search where to add some code later.

If you think it can be dangerous to keep the comment, I'll remove it.


Best regards

Laurent



Le 05/06/2024 à 20:13, Andreas Grabher a écrit :
> Thank you for fixing this bug. Btw. this did not just cause a warning, the code would have failed.
>
> In the old code m_reg was uint16_t. It probably makes sense to check the new code for other signed/unsigned data type errors. These errors are very hard to debug if discovered later.
>
> And I suggest to remove the commented lines.
>
> > Am 03.06.2024 um 23:02 schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
> >
> > Hello,
> >
> > thank you for the patch! I added it and have one minor issue:
> >
> > For this line in dsp_update_rn() I get a warning "Result of comparison of constant 65535 with expression of type 'int16_t' (aka 'short') is always false":
> >
> > if (reg_modulo == 65535) {
> >
> > Obviously the value is not recognized as -1 by the compiler. So probably better use -1 instead.
> >
> > Best wishes,
> > Andreas
> >
> > > Am 03.06.2024 um 22:30 schrieb Laurent Sallafranque <laurent.sallafranque@xxxxxxx>:
> > >
> > > Hi,
> > >
> > > First, I'm pleased to see that tuxfamily.org is back.
> > > I've taken some days to try to figure out why underscore demo was always bugging at the same point.
> > > The main explanation was the lack of pipeline emulation in the AGU.
> > >
> > > For example :
> > >     move #<10,r0
> > >     move #<15,r0
> > >     move x:(r0),a
> > >
> > >    The register "a" should equal 10 and not 15, due to a 1 instruction delay.
> > >    This happens for parallel moves instructions.
> > >
> > > I've implemented this behavior for all Rx, Nx and Mx registers.
> > > I've done a lot of non regression tests with many demos, programs, musik players, ... and didn't noticed any regression.
> > >
> > >
> > > I've seen this pipeline restriction in some demos with mod player sound embedded, with EKO System, and probably some other demos.
> > > The only one I know that suffered this was underscore demo.
> > >
> > > _ demo now renders the arrow correctly, with the blue lightened metallic texture and runs more frames before crashing. The crash doesn't always occur at the same point : sometimes it crashs before or later.
> > >
> > > The max I can now run is until this frame :
> > >
> > > <S2tblOwg38Bq3wQz.png>
> > >
> > >
> > > I've implemented the pipe for LUA, Parallel moves, MOVEC, TCC, MOVEP MOVE instructions.
> > > The motorola DSP docs also include MOVEM, but I've done many tests and it doesn't seem to suffer the pipeline restriction.
> > >
> > >
> > > This improvement in AGU emulation allows to feed the registers R, N or M just in time (also if this is not recommanded by Motorola ;)
> > >
> > > Regards
> > > Laurent