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