[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