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

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 05.06.2024 um 23:19 schrieb 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.

65535 is outside the range of a signed 16-bit integer. Therefore this compare would have never evaluated to true. 
> 
> 
>> 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.

Sounds like a good solution.
> 
> 
>> 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.

I‘m fine with that. Probably use C-style comment i stead of // to be in line with the rest of the file. 
> 
> 
> 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
>>>> 
>>> 
>>> 
>> 
> 
>