On 06/02/2019 03:37, Richard Henderson wrote:
> On 2/5/19 9:29 PM, Mark Cave-Ayland wrote:
>> The only minor question I had with the patchset in its current form is
>> whether to use
>> the new VsrD() macro for vscr_sat, or whether we don't really care enough?
>
> Given the comment
>
> /* Which bit we set is completely arbitrary, but clear the rest. */
>
> I don't think VsrD is helpful.
Okay, I can leave that for now.
> In "target/ppc: Split out VSCR_SAT to a vector field":
>
> ppc_vsr_t vsr[64] QEMU_ALIGNED(16);
> + /* Non-zero if and only if VSCR_SAT should be set. */
> + ppc_vsr_t vscr_sat;
>
> Better to add the QEMU_ALIGNED(16) to vscr_sat as well. Yes, it will already
> happen to be aligned by placement, but this is also a bit documentation.
I've added this to my latest branch.
> In "target/ppc: convert vadd*s and vsub*s to vector operations":
>
> if (sat) { \
> - set_vscr_sat(env); \
> + vscr_sat->u32[0] = 1; \
> } \
>
> Changed in error?
It looks like this was in the original patch, presumably because GEN_VXFORM_SAT
doesn't include the env parameter which was present in GEN_VXFORM_ENV. Should
the env
parameter be added to GEN_VXFORM_SAT?
Howard has also pointed out that he's still spotted some corruption in his
tests, so
I will do a bit more investigation and report back.
ATB,
Mark.
