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
