Re: [PATCH 11/18] rs6000: Builtin expansion, part 6

[Bill Schmidt <wschmidt@linux.ibm.com>]

Hi Segher,

Thank you for all of the reviews!  I appreciate your hard work and thorough 
study of the patches.

I've updated these 6 patches and combined them into 1, pushed today.  There 
are still a couple of cleanups I haven't done, but I made note in the code
where these are needed.

Thanks again!
Bill

On 11/3/21 8:24 PM, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Sep 01, 2021 at 11:13:47AM -0500, Bill Schmidt wrote:
>> Provide replacements for htm_spr_num and htm_expand_builtin.  No logic
>> changes are intended here, as usual.  Much code was factored out into
>> rs6000_expand_new_builtin, so the new version of htm_expand_builtin is
>> a little tidier.
> Nice.
>
>> Also implement the support for the "endian" and "32bit" attributes,
>> which is straightforward.  These just do icode substitution.
> Don't call this "attributes" please?  I don't know what would be a
> better name, of course.  "bif attribute" maybe?
>
>> +  rtx op[MAX_HTM_OPERANDS], pat;
> Don't declare arrays and scalars in the same statement, in general.  It
> is important that the arrays stand out.
>
> Also, don't declare things before they are used please.
>
>> +  FOR_EACH_CALL_EXPR_ARG (arg, iter, exp)
>> +    {
>> +      if (arg == error_mark_node || nopnds >= MAX_HTM_OPERANDS)
>> +	return const0_rtx;
>> +
>> +      insn_op = &insn_data[icode].operand[nopnds];
>> +      op[nopnds] = expand_normal (arg);
>> +
>> +      if (!insn_op->predicate (op[nopnds], insn_op->mode))
>> +	{
>> +	  if (!strcmp (insn_op->constraint, "n"))
>> +	    {
>> +	      int arg_num = (nonvoid) ? nopnds : nopnds + 1;
> Please don't parenthesise random expressions like "nonvoid".  I wonder
> if that can be simpler handled by just unshifting a void_node into the
> operands, btw :-)
>
> And the same "n" thing as before of course.  Since it is the same: some
> factoring would be helpful probably.
>
>> +      machine_mode mode = (TARGET_POWERPC64) ? DImode : SImode;
> Superfluous parens.  This is just "word_mode", anyway?
>
>> +  /* If this builtin accesses a CR, then pass in a scratch
>> +     CR as the last operand.  */
>> +  else if (bif_is_htmcr (*bifaddr))
>> +    {
>> +      cr = gen_reg_rtx (CCmode);
>> +      op[nopnds++] = cr;
>> +    }
> There is only one CR ("condition register").  You can say CRF here
> ("condition register field", a 4-bit thing), or just cc or CC maybe
> ("condition code").  A pet peeve, I know.
>
>> +  if (bif_is_endian (*bifaddr) && BYTES_BIG_ENDIAN)
> "is_endian" should maybe be "is_bigendian" or something like that?
>
> Okay for trunk with the changes you see fit at this time.  Thanks!
>
>
> Segher