Re: [PATCH 09/18] rs6000: Builtin expansion, part 4

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Wed, Sep 01, 2021 at 11:13:45AM -0500, Bill Schmidt wrote:
>  static insn_code
>  elemrev_icode (rs6000_gen_builtins fcode)
>  {
> +  switch (fcode)
> +    {
> +    default:
> +      gcc_unreachable ();

default: goes at the end.

> +    case RS6000_BIF_ST_ELEMREV_V1TI:
> +      return BYTES_BIG_ENDIAN
> +	? CODE_FOR_vsx_store_v1ti
> +	: CODE_FOR_vsx_st_elemrev_v1ti;

That fits on one or two lines.  Many more like that, I won't point them
all out.

Alternatively, maybe nicer, put an "if (BYTES_BIG_ENDIAN)" before the
switch, and duplicate the switch.  It probably is more readable that
way, easier to spot if you missed or typoedsomething.

> +    }
> +  gcc_unreachable ();

So the default: has no use at all!  We do the same after the switch
already.

>    return (insn_code) 0;

And neither does this line.  Leave it out, the gcc_unreachable tells GCC
that this path does not have to return a value (because this path can
never be taken).  Put a blank line before the gcc_unreachable though,
nice and dramatic right before the function-closing curly ;-)

>  static rtx
>  ldv_expand_builtin (rtx target, insn_code icode, rtx *op, machine_mode tmode)
>  {
> +  rtx pat, addr;
> +  bool blk = (icode == CODE_FOR_altivec_lvlx
> +	      || icode == CODE_FOR_altivec_lvlxl
> +	      || icode == CODE_FOR_altivec_lvrx
> +	      || icode == CODE_FOR_altivec_lvrxl);

"blk" is used 32 lines later.  Maybe a better name would help?  Maybe
you can declare (and init) the variable later?  Maybe a tiny comment
will help?

Apparently it means to use BLKmode instead of tmode, but why is that?

> +  if (target == 0
> +      || GET_MODE (target) != tmode
> +      || !insn_data[icode].operand[0].predicate (target, tmode))
> +    target = gen_reg_rtx (tmode);

And here it uses tmode anyway.  Hrm.

> +  /* For LVX, express the RTL accurately by ANDing the address with -16.
> +     LVXL and LVE*X expand to use UNSPECs to hide their special behavior,
> +     so the raw address is fine.  */

In the case of lvxl the unspec is not around the memory address, so this
is not true.

Oh, for lve* the same:

(define_insn "altivec_lve<VI_char>x"
  [(parallel
    [(set (match_operand:VI 0 "register_operand" "=v")
          (match_operand:VI 1 "memory_operand" "Z"))
     (unspec [(const_int 0)] UNSPEC_LVE)])]
  "TARGET_ALTIVEC"
  "lve<VI_char>x %0,%y1"
  [(set_attr "type" "vecload")])

The "set" is just plain.  It is paralleled with an unspec so it is not
the same RTL as some other load, but it is perfectly visible to all RTL
optimisers.

There needs to be an unspec in the "set" itself, like already done for
stve*: lve* leaves most of the vector undefined, but the RTL does not
express that currently, and then things can go wrong.  I cannot think of
an example where it *will* go wrong, but that does not say much :-)

So, all VMX-style loads and stores need the &-16 .

We survived this for ages, and it is not like lve* is such a hotly used
builtin these days, so we'll survive things, but: put it on a to-do
list somewhere?  :-)

> +  /* Emit the lxvr*x insn.  */
> +  pat = GEN_FCN (icode) (tiscratch, addr);

(declare it here, "rtx pat", not much earlier)

Okay for trunk with whatever tidyings you feel you can make now, and
leave the rest for a later day.  Thanks!


Segher