On Apr 28, 2020, Segher Boessenkool <seg...@kernel.crashing.org> wrote:
> s/mmfl/mffs/
> s/mmfs/mffs/
> s/mmsl/mffsl/
Oh, my, looks like I missed some mispellings of ffmls :-P
Sorry about the typos, for some reason mffs makes it worse than usual
for me.
> Okay for trunk with such tweaks. Thank you! Also okay for the 9 branch,
> after waiting to see if any fallout appears.
Thanks, here's what I'm checking in...
[rs6000] fix mffsl emulation
From: Alexandre Oliva <ol...@adacore.com>
The emulation of mffsl with mffs, used when !TARGET_P9_MISC, is going
through the motions, but not storing the result in the given
operands[0]; it rather modifies operands[0] without effect. It also
creates a DImode pseudo that it doesn't use, overwriting subregs
instead.
The patch below fixes all of these, the indentation and a typo.
I'm concerned about several issues in the mffsl testcase. First, I
don't see that comparing the values as doubles rather than as long
longs is desirable. These are FPSCR bitfields, not FP numbers. I
understand mffs et al use double because they output to FP registers,
and the bit patterns are subnormal FP numbers, so it works, but given
the need for bit masking of at least one side, I'm changing the
compare to long longs.
Another issue with the test is that, if the compare fails, it calls
mffsl again to print the value, as if it would yield the same result.
But part of the FPSCR that mffsl (emulated with mffs or not) copies to
the output FP register is the FPCC, so the fcmpu used to compare the
result of the first mffsl will modify FPSCR and thus the result of the
second mffsl call. After changing the compare, this is no longer the
case, but I still think it's better to make absolutely sure what we
print is what we compared.
Yet another issue is that the test assumed the mffs bits that are not
to be extracted by mffsl to be already zero, instead of masking them
out explicitly. This is not about the mffs emulation in the mffsl
implementation, but about the mffs use in the test proper. The bits
appear to be zero indeed, as the bits left out are for sticky
exceptions, but there are reserved parts of FPSCR that might turn out
to be set in the future, so we're better off masking them out
explicitly, otherwise those bits could cause the compare to fail.
If some future mffsl is changed so that it copies additional nonzero
bits, the test will fail, and then we'll have a chance to adjust it
and the emulation.
for gcc/ChangeLog
PR target/94812
* gcc/config/rs6000/rs6000.md (rs6000_mffsl): Copy result to
output operand in emulation. Don't overwrite pseudos.
for gcc/testsuite/ChangeLog
PR target/94812
* gcc.target/powerpc/test_mffsl.c: Call mffsl only once.
Reinterpret the doubles as long longs for compares. Mask out
mffs bits that are not expected from mffsl.
---
gcc/config/rs6000/rs6000.md | 25 +++++++++++++------------
gcc/testsuite/gcc.target/powerpc/test_mffsl.c | 12 ++++++++----
2 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 11ab745..6173994 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -13620,18 +13620,19 @@
if (!TARGET_P9_MISC)
{
- rtx tmp_di = gen_reg_rtx (DImode);
- rtx tmp_df = gen_reg_rtx (DFmode);
-
- /* The mffs instruction reads the entire FPSCR. Emulate the mffsl
- instruction using the mffs instruction and masking off the bits
- the mmsl instruciton actually reads. */
- emit_insn (gen_rs6000_mffs (tmp_df));
- tmp_di = simplify_gen_subreg (DImode, tmp_df, DFmode, 0);
- emit_insn (gen_anddi3 (tmp_di, tmp_di, GEN_INT (0x70007f0ffLL)));
-
- operands[0] = simplify_gen_subreg (DFmode, tmp_di, DImode, 0);
- DONE;
+ rtx tmp1 = gen_reg_rtx (DFmode);
+
+ /* The mffs instruction reads the entire FPSCR. Emulate the mffsl
+ instruction using the mffs instruction and masking the result. */
+ emit_insn (gen_rs6000_mffs (tmp1));
+
+ rtx tmp1di = simplify_gen_subreg (DImode, tmp1, DFmode, 0);
+ rtx tmp2 = gen_reg_rtx (DImode);
+ emit_insn (gen_anddi3 (tmp2, tmp1di, GEN_INT (0x70007f0ffLL)));
+
+ rtx tmp2df = simplify_gen_subreg (DFmode, tmp2, DImode, 0);
+ emit_move_insn (operands[0], tmp2df);
+ DONE;
}
emit_insn (gen_rs6000_mffsl_hw (operands[0]));
diff --git a/gcc/testsuite/gcc.target/powerpc/test_mffsl.c
b/gcc/testsuite/gcc.target/powerpc/test_mffsl.c
index 93a8ec2..41377ef 100644
--- a/gcc/testsuite/gcc.target/powerpc/test_mffsl.c
+++ b/gcc/testsuite/gcc.target/powerpc/test_mffsl.c
@@ -14,17 +14,21 @@ int main ()
union blah {
double d;
unsigned long long ll;
- } conv_val;
+ } mffs_val, mffsl_val;
/* Test reading the FPSCR register. */
__asm __volatile ("mffs %0" : "=f"(f14));
- conv_val.d = f14;
+ mffs_val.d = f14;
+ /* Select the same bits as mffsl. */
+ mffs_val.ll &= 0x70007f0ffLL;
- if (conv_val.d != __builtin_mffsl())
+ mffsl_val.d = __builtin_mffsl ();
+
+ if (mffs_val.ll != mffsl_val.ll)
{
#ifdef DEBUG
printf("ERROR, __builtin_mffsl() returned 0x%llx, not the expecected
value 0x%llx\n",
- __builtin_mffsl(), conv_val.d);
+ mffsl_val.ll, mffs_val.ll);
#else
abort();
#endif
--
Alexandre Oliva, freedom fighter he/him https://FSFLA.org/blogs/lxo/
Free Software Evangelist Stallman was right, but he's left :(
GNU Toolchain Engineer Live long and free, and prosper ethically
