Re: [Qemu-devel] [PATCH 2/2] target-mips: Implement IEEE 754-2008 functionality for R6 and MSA instructions

["Aleksandar Markovic" <Aleksandar.Markovic@rt-rk.com>]

[-- Attachment #1: Type: text/plain, Size: 4849 bytes --]


Hello, Leon, thank you very much for the kind feedback. Let me clarify my take on the involved issues.

1) Class operations

I am going to correct the code as you hinted.

The reason I wanted separate handling of MSA class operation is code and module decoupling. Handling of MSA instructions (in file msa_helper.c) and regular instructions (in file op_helper.c) have many overlaping areas - however, my understanding is that the designer of MSA module wanted it to be as independant on code in other files/modulas as possible. Handling class operation is on of the rare instances where code in msa_helper.c relies on the code in op_helper.c., and it made sense to me that this dependence should be removed, for the sake of consistency within MSA module - even if the functionalitied are virtually identical. That said, I will anyway listen to your advice, since you most probably see more than myself regarding this, and I am going to revert to a single handling of class operations, for both MSA and regular versions.

2) Flush subnormals

My impression is that his set of features should be treated and implemented separately, at some later point in time.

Although the implementation seems not to be too complex (defining FCR31_FS, invoking appropriately set_flush_to_zero() and set_flush_inputs_to_zero() on CPU init, plus special exception handling, like it is already done for MSA equivalents), it looks to me that it would have added a lot of risk into a patch series that is already touching a lot of sensitive areas, and therefore introducing a lot of risks. Once this patch series is hopefully intergrated, flush subnormals will be much easier to integrate, since it will be mips-only issue. Therefore, if you agree, I will leave it for the future. I will definitely mention it in commit messages though (as a limitaion), for future reference.

Thanks again for your consideration of this matter.

Sincerely yours,
Aleksandar


-------- Original Message --------
Subject: Re: [PATCH 2/2] target-mips: Implement IEEE 754-2008 functionality for R6 and MSA instructions
Date: Friday, April 1, 2016 21:07 CEST
From: Leon Alrae <leon.alrae@imgtec.com>
To: Aleksandar Markovic <aleksandar.markovic@rt-rk.com>,<qemu-devel@nongnu.org>
CC: <qemu-arm@nongnu.org>, <qemu-ppc@nongnu.org>, <aurelien@aurel32.net>,<peter.maydell@linaro.org>, <rth@twiddle.net>, <afaerber@suse.de>,<pbonzini@redhat.com>, <ehabkost@redhat.com>, <edgar.iglesias@gmail.com>,<proljc@gmail.com>, <agraf@suse.de>, <blauwirbel@gmail.com>,<mark.cave-ayland@ilande.co.uk>, <gxt@mprc.pku.edu.cn>,<petar.jovanovic@imgtec.com>, <miodrag.dinic@imgtec.com>,<jcmvbkbc@gmail.com>, <kbastian@mail.uni-paderborn.de>
References: <1458910214-12239-1-git-send-email-aleksandar.markovic@rt-rk.com><1458910214-12239-3-git-send-email-aleksandar.markovic@rt-rk.com>


 On 25/03/16 12:50, Aleksandar Markovic wrote:
> +#define MSA_CLASS_SIGNALING_NAN 0x001
> +#define MSA_CLASS_QUIET_NAN 0x002
> +#define MSA_CLASS_NEGATIVE_INFINITY 0x004
> +#define MSA_CLASS_NEGATIVE_NORMAL 0x008
> +#define MSA_CLASS_NEGATIVE_SUBNORMAL 0x010
> +#define MSA_CLASS_NEGATIVE_ZERO 0x020
> +#define MSA_CLASS_POSITIVE_INFINITY 0x040
> +#define MSA_CLASS_POSITIVE_NORMAL 0x080
> +#define MSA_CLASS_POSITIVE_SUBNORMAL 0x100
> +#define MSA_CLASS_POSITIVE_ZERO 0x200
> +
> +#define MSA_CLASS(name, bits) \
> +uint ## bits ## _t helper_msa_ ## name (CPUMIPSState *env, \
> + uint ## bits ## _t arg) \
> +{ \
> + if (float ## bits ## _is_signaling_nan(arg, \
> + &env->active_tc.msa_fp_status)) { \
> + return MSA_CLASS_SIGNALING_NAN; \
> + } else if (float ## bits ## _is_quiet_nan(arg, \
> + &env->active_tc.msa_fp_status)) { \
> + return MSA_CLASS_QUIET_NAN; \
> + } else if (float ## bits ## _is_neg(arg)) { \
> + if (float ## bits ## _is_infinity(arg)) { \
> + return MSA_CLASS_NEGATIVE_INFINITY; \
> + } else if (float ## bits ## _is_zero(arg)) { \
> + return MSA_CLASS_NEGATIVE_ZERO; \
> + } else if (float ## bits ## _is_zero_or_denormal(arg)) { \
> + return MSA_CLASS_NEGATIVE_SUBNORMAL; \
> + } else { \
> + return MSA_CLASS_NEGATIVE_NORMAL; \
> + } \
> + } else { \
> + if (float ## bits ## _is_infinity(arg)) { \
> + return MSA_CLASS_POSITIVE_INFINITY; \
> + } else if (float ## bits ## _is_zero(arg)) { \
> + return MSA_CLASS_POSITIVE_ZERO; \
> + } else if (float ## bits ## _is_zero_or_denormal(arg)) { \
> + return MSA_CLASS_POSITIVE_SUBNORMAL; \
> + } else { \
> + return MSA_CLASS_POSITIVE_NORMAL; \
> + } \
> + } \
> +}

Duplicating the class operation is unnecessary. We can just have common
function for FPU and MSA which takes additional float_status argument.

Also I noticed that this patch series doesn't provide Flush Subnormals
(the FCSR.FS bit), but probably this functionality can come later...

Leon


 

[-- Attachment #2: Type: text/html, Size: 5565 bytes --]