Re: [Qemu-devel] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>

[Leon Alrae]

On Fri, Jun 10, 2016 at 09:12:12PM +0100, Maciej W. Rozycki wrote:
> On Fri, 10 Jun 2016, Aleksandar Markovic wrote:
> 
> > The changes that make QEMU behavior the same as hardware behavior (in 
> > relation to CEIL, CVT, FLOOR, ROUND, TRUNC Mips instructions) are 
> > already contained in this patch.
> 
>  Good, however that means that you've really combined two logically 
> separate changes into a single patch:
> 
> 1. A bug fix for SoftFloat legacy-NaN (original) MIPS support, which has 
>    been there probably since forever (i.e. since the MIPS target was added 
>    to QEMU).

I've just done another round of review and as far as I can tell these
patches don't modify the legacy-NaN MIPS behaviour. I believe Aleksandar
was referring to new functionality (i.e. 2008 NaN) only.

Regards,
Leon

> 
> 2. A new feature for 2008-NaN MIPS support.
> 
> To me it really looks like the two need to be separate patches, with the 
> bug fix applied first (or among any other bug fixes at the beginning) in 
> the patch set, or even as a separate change marked as a prerequisite for 
> the rest of the changes.
> 
>  The bug fix will then be self-contained and more prominently exposed, 
> rather than being buried among feature additions.  It can then be 
> independently reviewed and likely more easily accepted as long as it is 
> technically correct.  It can also be cherry-picked and backported easily 
> if necessary, perhaps outside the upstream tree.
> 
>  Review of the new feature set can then follow, once the bug(s) have been 
> fixed.
> 
> > I just mentioned Mips-A / Mips-B / SoftFloat differences as an 
> > explanation/observation related to the change in this patch.
> 
>  Maybe it's just myself, but from your description I got the impression 
> that your change preserves the status quo and the explanation merely 
> serves the purpose of documenting it.  Please consider rewriting it such 
> that it is unambiguous that the SoftFloat bug is being fixed with your 
> change.
> 
>  Obviously once you've made the bug fix a separate change, it'll become 
> unambiguous naturally, as then you won't have the 2008-NaN feature along 
> it obfuscating the picture.
> 
>   Maciej