[PATCH][arm][1/X] Add initial support for saturation intrinsics
Richard Henderson
richard.henderson@linaro.org
Sat Nov 9 13:49:00 GMT 2019
> +;; define_subst and associated attributes
> +
> +(define_subst "add_setq"
> + [(set (match_operand:SI 0 "" "")
> + (match_operand:SI 1 "" ""))]
> + ""
> + [(set (match_dup 0)
> + (match_dup 1))
> + (set (reg:CC APSRQ_REGNUM)
> + (unspec:CC [(reg:CC APSRQ_REGNUM)] UNSPEC_Q_SET))])
> +
> +(define_subst_attr "add_clobber_q_name" "add_setq" "" "_setq")
> +(define_subst_attr "add_clobber_q_pred" "add_setq" "!ARM_Q_BIT_READ"
> + "ARM_Q_BIT_READ")
Is there a good reason to use CCmode for the Q bit instead of BImode?
Is there a good reason not to represent the clobber of the Q bit when we're not
representing the set?
Although it probably doesn't matter, because of the unspec, the update of the Q
bit here in the subst isn't right. Better would be
(set (reg:BI APSRQ_REGNUM)
(ior:BI (unspec:BI [(match_dup 1)] UNSPEC_Q_SET)
(reg:BI APSRQ_REGNUM)))
> +/* Implement TARGET_CHECK_BUILTIN_CALL. Record a read of the Q bit through
> + intrinsics in the machine function. */
> +bool
> +arm_check_builtin_call (location_t , vec<location_t> , tree fndecl,
> + tree, unsigned int, tree *)
> +{
> + int fcode = DECL_MD_FUNCTION_CODE (fndecl);
> + if (fcode == ARM_BUILTIN_saturation_occurred
> + || fcode == ARM_BUILTIN_set_saturation)
> + {
> + if (cfun && cfun->decl)
> + DECL_ATTRIBUTES (cfun->decl)
> + = tree_cons (get_identifier ("acle qbit"), NULL_TREE,
> + DECL_ATTRIBUTES (cfun->decl));
> + }
> + return true;
> +}
Where does this attribute affect inlining? Or get merged into the signature of
a calling function during inlining?
This check happens way way early in the c front end, while doing semantic
checks. I think this is the wrong hook to use, especially since this is not a
semantic check of the arguments to the builtin.
I think you want to record the use of such a builtin during rtl expansion,
within the define_expand for the buitin, setting a bool in struct
machine_function. Then use that bool in the arm.md predicates.
(I'll note that there are 5 "int" fields in the arm machine_function that
should be bool, now that we're not using C89. Probably best to re-order all of
them to the end and add your new bool next to them.)
> +(define_insn "arm_set_apsr"
> + [(set (reg:CC APSRQ_REGNUM)
> + (unspec_volatile:CC
> + [(match_operand:SI 0 "s_register_operand" "r")] VUNSPEC_APSR_WRITE))]
> + "TARGET_ARM_QBIT"
> + "msr%?\tAPSR_nzcvq, %0"
> + [(set_attr "predicable" "yes")
> + (set_attr "conds" "set")]
> +)
This is missing a clobber (or set) of CC_REGNUM.
Why unspec_volatile and not unspec?
> +;; Read the APSR and set the Q bit (bit position 27) according to operand 0
> +(define_expand "arm_set_saturation"
> + [(match_operand:SI 0 "reg_or_int_operand")]
> + "TARGET_ARM_QBIT"
> + {
> + rtx apsr = gen_reg_rtx (SImode);
> + emit_insn (gen_arm_get_apsr (apsr));
> + rtx to_insert = gen_reg_rtx (SImode);
> + if (CONST_INT_P (operands[0]))
> + emit_move_insn (to_insert, operands[0] == CONST0_RTX (SImode)
> + ? CONST0_RTX (SImode) : CONST1_RTX (SImode));
> + else
> + {
> + rtx cmp = gen_rtx_NE (SImode, operands[0], CONST0_RTX (SImode));
> + emit_insn (gen_cstoresi4 (to_insert, cmp, operands[0],
> + CONST0_RTX (SImode)));
> + }
> + emit_insn (gen_insv (apsr, CONST1_RTX (SImode),
> + gen_int_mode (27, SImode), to_insert));
> + emit_insn (gen_arm_set_apsr (apsr));
> + DONE;
> + }
> +)
Why are you preserving APSR.NZCV across this operation? It should not be live
during the builtin that expands this.
r~
More information about the Gcc-patches
mailing list