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
