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

[Maciej W. Rozycki]

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).

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