[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|C
From: |
Leon Alrae |
Subject: |
Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D> |
Date: |
Tue, 14 Jun 2016 15:17:53 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Maciej W. Rozycki, 2016/06/07
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Aleksandar Markovic, 2016/06/10
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Maciej W. Rozycki, 2016/06/10
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Aleksandar Markovic, 2016/06/10
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Maciej W. Rozycki, 2016/06/10
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>,
Leon Alrae <=
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Maciej W. Rozycki, 2016/06/14
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Aleksandar Markovic, 2016/06/20
- Re: [Qemu-arm] [PATCH v6 8/9] target-mips: Add nan2008 flavor of <CEIL|CVT|FLOOR|ROUND|TRUNC>.<L|W>.<S|D>, Maciej W. Rozycki, 2016/06/20