[PATCH 06/18] rs6000: Builtin expansion, part 1

Segher Boessenkool segher@kernel.crashing.org
Sun Oct 31 03:24:36 GMT 2021


Hi!

On Wed, Sep 01, 2021 at 11:13:42AM -0500, Bill Schmidt wrote:
> Differences between the old and new support in this patch include:
>  - Make use of the new builtin data structures, directly looking up
>    a function's information rather than searching for the function
>    multiple times;

Is that measurable, do you think?

>  - Test for enablement of builtins at expand time, to support #pragma
>    target changes within a compilation unit;

But not within a function, right?

> Note that these six patches must be pushed together, because otherwise
> unused parameter warnings in the stub functions will prevent bootstrap.

Must be *committed* as one commit, even.  Committing them as six
separate commits and pushing them all at once will do nothing for people
who try to bisect, etc.

Merging patches is easy with Git though :-)

> +/* Expand ALTIVEC_BUILTIN_MASK_FOR_LOAD.  */
> +rtx
> +rs6000_expand_ldst_mask (rtx target, tree arg0)
> + {
> +  return target;
> + }

Interesting leading spaces, heh.  Please fix.

> +/* Expand the HTM builtin in EXP and store the result in TARGET.  */
> +static rtx
> +new_htm_expand_builtin (bifdata *bifaddr, rs6000_gen_builtins fcode,
> +			tree exp, rtx target)
> +{
> +  return const0_rtx;
> +}

The function commment should say what the return value is.

> +/* Expand an expression EXP that calls a built-in function,
> +   with result going to TARGET if that's convenient
> +   (and in mode MODE if that's convenient).
> +   SUBTARGET may be used as the target for computing one of EXP's operands.
> +   IGNORE is nonzero if the value is to be ignored.
> +   Use the new builtin infrastructure.  */
> +static rtx
> +rs6000_expand_new_builtin (tree exp, rtx target,
> +			   rtx subtarget ATTRIBUTE_UNUSED,
> +			   machine_mode ignore_mode ATTRIBUTE_UNUSED,
> +			   int ignore ATTRIBUTE_UNUSED)

Don't use ATTRIBUTE_UNUSED?  We have C++ now, you can leave out the
parameter name, with the same effect (other than it does not make you go
blind ;-) ).  In the case where you still want to show the name, you can
do something like
rs6000_expand_new_builtin (tree exp, trx target, rtx /*subtarget*/,
			    machine_mode, int /*ignore*/)

(There is no argument MODE btw, the comment needs some tweaking).

> +  /* We have two different modes (KFmode, TFmode) that are the IEEE
> +     128-bit floating point type, depending on whether long double is the
> +     IBM extended double (KFmode) or long double is IEEE 128-bit (TFmode).

KFmode *always* is IEEE QP.  TFmode is the one that can be different.

> +     It is simpler if we only define one variant of the built-in function,
> +     and switch the code when defining it, rather than defining two built-
> +     ins and using the overload table in rs6000-c.c to switch between the
> +     two.  If we don't have the proper assembler, don't do this switch
> +     because CODE_FOR_*kf* and CODE_FOR_*tf* will be CODE_FOR_nothing.  */
> +  if (FLOAT128_IEEE_P (TFmode))
> +    switch (icode)
> +      {
> +      default:
> +	break;

default: goes at the *end*.  And you can usually leave it out.

> +      case CODE_FOR_sqrtkf2_odd:
> +	icode = CODE_FOR_sqrttf2_odd;
> +	break;

So please do this the other way?  In libgcc "tf" means double-double,
it is historical.  So let's do the clearer thing please: translate tf to
kf in this handling (when tf *does* mean kf ;-) )

> +  /* In case of "#pragma target" changes, we initialize all builtins
> +     but check for actual availability now, during expand time.  For
> +     invalid builtins, generate a normal call.  */
> +  bifdata *bifaddr = &rs6000_builtin_info_x[uns_fcode];
> +  bif_enable e = bifaddr->enable;
> +
> +  if (e != ENB_ALWAYS
> +      && (e != ENB_P5       || !TARGET_POPCNTB)

  if (!(e == ENB_ALWAYS
	|| (e == ENB_P5 && TARGET_POPCNTB)

etc.

Computers are better at De Morgan than humans are.  It is much more
important to write clear code.  This often means using fewer negations.

> +  const int MAX_BUILTIN_ARGS = 6;
> +  tree arg[MAX_BUILTIN_ARGS];
> +  rtx op[MAX_BUILTIN_ARGS];
> +  machine_mode mode[MAX_BUILTIN_ARGS + 1];

Arrays are always better with a short comment.
Why is the "mode" array one entry longer, btw?

> +  rtx pat;
> +  bool void_func = TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node;
> +  int k;

These things should be declared where they are first used.  The
initialisation of void_func should not be hidden amidst boring
declarations.

"k" needs a comment.

> +      if (!insn_data[icode].operand[i+k].mode)
> +	mode[i+k] = TARGET_64BIT ? Pmode : SImode;

That is
  mode[i+k] = Pmode;
always.

Does this depend on VOIDmode being equal to 0?  That is guaranteed, but
if you write out VOIDmode elsewhere, do it here as well?

> +      else
> +	mode[i+k] = insn_data[icode].operand[i+k].mode;
> +    }

So this is
  mode[i+k] = insn_data[icode].operand[i+k].mode;
  if (!mode[i+k])
    mode[i+k] = Pmode;

> +      switch (bifaddr->restr[i])
> +	{
> +	default:

default: goes at the end.

> +	case RES_BITS:
> +	  {
> +	    size_t mask = (1 << bifaddr->restr_val1[i]) - 1;

1 is an int, it can overflow much before a size_t would.
  size_t mask = 1;
  mask <<= bifaddr->restr_val1[i];
  mask--;

> +	    tree restr_arg = arg[bifaddr->restr_opnd[i] - 1];
> +	    STRIP_NOPS (restr_arg);
> +	    if (TREE_CODE (restr_arg) != INTEGER_CST
> +		|| TREE_INT_CST_LOW (restr_arg) & ~mask)

Manual De Morgan again?  More later too, ugh.  Well at least these
are trivial either way.

> +	case RES_VAR_RANGE:
> +	  {
> +	    tree restr_arg = arg[bifaddr->restr_opnd[i] - 1];
> +	    STRIP_NOPS (restr_arg);
> +	    if (TREE_CODE (restr_arg) == INTEGER_CST
> +		&& !IN_RANGE (tree_to_shwi (restr_arg),
> +			      bifaddr->restr_val1[i],
> +			      bifaddr->restr_val2[i]))
> +	      {
> +		error ("argument %d must be a variable or a literal "
> +		       "between %d and %d, inclusive",
> +		       bifaddr->restr_opnd[i], bifaddr->restr_val1[i],
> +		       bifaddr->restr_val2[i]);
> +		return CONST0_RTX (mode[0]);
> +	      }
> +	    break;
> +	  }

This error check is incongruent with the rest, and with its error
message?  If it is not an INTEGER_CST, it does not check anything about
it.  That sounds like trouble later.

> +  if (fcode == RS6000_BIF_PACK_IF
> +      && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)

Curious line breaks.  Break before each &&, or don't break before the
first one?

> +    {
> +      icode = CODE_FOR_packtf;
> +      fcode = RS6000_BIF_PACK_TF;

The fcode was for IF, can you use TF now?

> +      uns_fcode = (size_t)fcode;
> +    }

(space after cast)

> +  else if (fcode == RS6000_BIF_UNPACK_IF
> +	   && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
> +    {
> +      icode = CODE_FOR_unpacktf;
> +      fcode = RS6000_BIF_UNPACK_TF;
> +      uns_fcode = (size_t)fcode;
> +    }

(same issues)

> +  switch (nargs)
> +    {
> +    default:

Just Say No.  Don't write  0 == err  and don't put default: first.  It
does not improve your code.  It makes it worse, instead.  Yoda can
understand Yoda-speak, it comes natural to him.  Yoda is not amongst the
readers of your code though, so please write in the common idiom :-)


This is okay for trunk with these things fixed.  Thanks!


Segher


More information about the Gcc-patches mailing list