From 50714f45eeaf315a0b55d3db3de3bf8df8e94b04 Mon Sep 17 00:00:00 2001 From: Alexandre Oliva Date: Thu, 23 Apr 2020 02:19:55 -0300 Subject: [PATCH] [rs6000] fix mffsl emulation 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/ChangeLog | 6 +++++ gcc/config/rs6000/rs6000.md | 25 ++++++++++--------- gcc/testsuite/ChangeLog | 7 ++++++ gcc/testsuite/gcc.target/powerpc/test_mffsl.c | 12 ++++++--- 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 9c947299964..b7ec1386740 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2020-04-28 Alexandre Oliva + + PR target/94812 + * gcc/config/rs6000/rs6000.md (rs6000_mffsl): Copy result to + output operand in emulation. Don't overwrite pseudos. + 2020-04-28 Jeff Law * config/h8300/h8300.md (H8/SX mult patterns): All H8/SX specific diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index 11ab74589b4..6173994797c 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/ChangeLog b/gcc/testsuite/ChangeLog index 74755cb23a0..d20308b376c 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2020-04-28 Alexandre Oliva + + 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. + 2020-04-28 David Malcolm PR analyzer/94816 diff --git a/gcc/testsuite/gcc.target/powerpc/test_mffsl.c b/gcc/testsuite/gcc.target/powerpc/test_mffsl.c index 93a8ec24516..41377efba1a 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 -- 2.30.2