Hi Carl,
Sorry for the late review.
On Mon, Sep 17, 2018 at 03:03:28PM -0700, Carl Love wrote:
> * config/rs6000.c (rs6000_expand_mtfsb0_mtfsb1_builtin): Add.
config/rs6000/rs6000.c
> +RS6000_BUILTIN_X (RS6000_BUILTIN_MTFSB0, "__builtin_mtfsb0",
> + RS6000_BTM_ALWAYS,
> + RS6000_BTC_MISC | RS6000_BTC_UNARY,
> + CODE_FOR_rs6000_mtfsb0)
I think you need RS6000_BTC_VOID on most of these calls?
> static rtx
> +rs6000_expand_mtfsb_builtin (enum insn_code icode, tree exp)
The changelog has the function name wrong?
> + pat = GEN_FCN (icode) (op0);
> + if (! pat)
No space after ! (or any other prefix operator that doesn't have letters
in its name, i.e. casts, sizeof, etc.)
> +static rtx
> +rs6000_expand_set_fpscr_rn_builtin (enum insn_code icode, tree exp)
> + /* If the argument is a constant, check the range. Argument can only be a
> + 2-bit value. Unfortunately, can't check the range of the value at
> + compile time if the argument is a variable. */
So what do we do for variable args? Mask off the bits?
> +static rtx
> +rs6000_expand_set_fpscr_drn_builtin (enum insn_code icode, tree exp)
> + /* If the argument is a constant, check the range. Agrument can only be a
> + 3-bit value. Unfortunately, can't check the range of the value at
> + compile time if the argument is a variable.
> + */
Don't put */ on a separate line please.
> + if (GET_CODE (op0) == CONST_INT && !const_0_to_7_operand(op0, VOIDmode ))
Stray space after VOIDmode.
> +(define_insn "rs6000_mtfsb0"
> + [(unspec_volatile [(match_operand:SI 0 "u5bit_cint_operand" "n")]
> + UNSPECV_MTFSB0)]
> + "TARGET_HARD_FLOAT"
> + "mtfsb0 %0"
> + [(set_attr "type" "fp")])
Hrm... Does all of this work with -msoft-float? Does it not ICE at least?
> +(define_expand "rs6000_set_fpscr_rn"
> + [(match_operand:DI 0 "gpc_reg_operand")]
(Two spaces.)
You could handle immediate operands separately: you need only two mtfsbN
instructions for that, which is smaller and faster than the "variable"
sequence. Well, for non-P9 anyway.
> +(define_expand "rs6000_mffsl"
> + [(set (match_operand:DF 0 "gpc_reg_operand")
> + (unspec_volatile:DF [(const_int 0)] UNSPECV_MFFSL))]
> + "TARGET_HARD_FLOAT"
> +{
> + /* If the low latency mffsl instruction (ISA 3.0) is available use it,
> + otherwise fall back to the older mffs instruction to emulate the mffsl
> + instruction. */
> +
> + if (TARGET_P9_MISC)
> + emit_insn (gen_rs6000_mffsl_hw (operands[0]));
The indent is incorrect. But you could just not do anything if
TARGET_P9_MISC: the RTL pattern above already is exactly what you need.
So "if (!TARGET_P9_MISC)" and then that block with the DONE moved in there,
and the TARGET_P9_MISC case can just fall through.
> + emit_insn (gen_anddi3 (tmp_di, tmp_di, GEN_INT (0x70007F0FFLL)));
Write numbers in lower case please (for readability). 0x70007f0ffLL
> +(define_insn "rs6000_mtfsf_hi"
> + [(unspec_volatile [(match_operand:SI 0 "const_int_operand" "i")
const_int_operand is "n": an actual number, not e.g. a constant address
(like "i" allows). It doesn't make a real difference in some cases, but
it's best to get it right.
> +the most significant word on 32-bit environments. The @code{__builtin_mffs}
> +return the value of the FPSCR register. Note, ISA 3.0 supports the
> +@code{__builtin_mffsl()} which is a lower latency version of this builtin.
> The
mffsl does not return the whole fpscr, just the more useful (and cheaper
to access!) fields: rn and drn, the exception enables, the non-sticky
exception flags.
> +++ b/gcc/testsuite/gcc.target/powerpc/test_fpscr_drn_builtin.c
> @@ -0,0 +1,116 @@
> +/* { dg-do run { target { powerpc*-*-linux* && lp64 } } } */
Why only linux?
Okay for trunk with the nits fixed; the things that are more like
extensions can happen later (if at all). Thanks!
Segher
