search

beardypig / ghidra-emotionengine

Ghidra Processor for the Play Station 2's Emotion Engine MIPS based CPU
Apache License 2.0
198 stars 35 forks source link

Fix parallel shift instructions. #43

Closed PFedak closed 3 years ago

PFedak commented 3 years ago

Per the spec, these operations should be equivalent to mov when the shift amount is zero. The diagrams are a little misleading.

PFedak commented 3 years ago

I'm not familiar enough with sleigh to know what the sa:4 etc. bits are doing/whether they are necessary. Similarly, I think the temp variable is needed to get the right 32-bit-truncation semantics for psllvw, but please let me know if there's an easier way.

PFedak commented 3 years ago

Actually I was mistaken. The sa should actually be fsa which is the shift amount register. As for why it is names fsa I don't fully remember other than sleigh wouldn't allow both a register to be named sa and to have the sa in the instruction token.

I'm not sure I understand, I thought fsa was a special register just for the funnel shift (qfsrv) instruction.

astrelsky commented 3 years ago

Actually I was mistaken. The sa should actually be fsa which is the shift amount register. As for why it is names fsa I don't fully remember other than sleigh wouldn't allow both a register to be named sa and to have the sa in the instruction token.

I'm not sure I understand, I thought fsa was a special register just for the funnel shift (qfsrv) instruction.

Yes you are correct. I apologize as I must have confused myself when looking at the opcode portions.

The sa:4 here is the shift amount specified by by bits 6-10 of the instruction. The :4 is just to set the varnode size of the shift amount so that it will cooperate.

PFedak commented 3 years ago

Ah, no problem, I feel like I always have to read the documentation three times before I'm sure I understand. I undid part of my change for psllvw so I think this is okay to merge unless you want additional changes.

Keep an eye out for another PR from me in a bit for some unrelated issues.

astrelsky commented 3 years ago

Ah, no problem, I feel like I always have to read the documentation three times before I'm sure I understand. I undid part of my change for psllvw so I think this is okay to merge unless you want additional changes.

Keep an eye out for another PR from me in a bit for some unrelated issues.

Ok no problem. I will give this another look in the morning with a clearer head in case I missed something. Hopefully @beardypig could also take a look as I can be a bit absent minded sometimes.

beardypig commented 3 years ago

Thanks for the contribution @PFedak!