Re: [PATCH v3] rs6000: Adjust mov optabs for opaque modes [PR103353]

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

Hi!

On Mon, Jun 27, 2022 at 10:47:26AM +0800, Kewen.Lin wrote:
> on 2022/6/25 00:49, Segher Boessenkool wrote:
> Many thanks for all the further explanation above!  The attached patch
> updated the misleading comments as you pointed out and suggested, could
> you help to have another look?

Please do not send proposed patches in the middle of a mail thread.  It
is harder to track things than needed this way, and it makes it hard to
apply your patches as well (for test builds, say).

> Subject: [PATCH] rs6000: Adjust mov optabs for opaque modes [PR103353]
> 
> As PR103353 shows, we may want to continue to expand built-in
> function __builtin_vsx_lxvp, even if we have already emitted
> error messages about some missing required conditions.  As
> shown in that PR, without one explicit mov optab on OOmode
> provided, it would call emit_move_insn recursively.

> -  rs6000_emit_move (operands[0], operands[1], OOmode);
> -  DONE;
> +  if (TARGET_MMA) {
> +    rs6000_emit_move (operands[0], operands[1], OOmode);
> +    DONE;
> +  }

  if (TARGET_MMA)
    {
      rs6000_emit_move (operands[0], operands[1], OOmode);
      DONE;
    }

> +  /* PR103353 shows we may want to continue to expand the __builtin_vsx_lxvp
> +     built-in function, even if we have already emitted error messages about
> +     some missing required conditions.  As shown in that PR, without one
> +     explicit mov optab on OOmode provided, it would call emit_move_insn
> +     recursively.  So we allow this pattern to be generated when we are
> +     expanding to RTL and have seen errors.  It would not cause further ICEs
> +     as the compilation would stop soon after expanding.  */
> +  else if (currently_expanding_to_rtl && seen_error ())
> +    ;

The comment goes inside this "else if" branch.  Maybe make it a braced
block if the semicolon looks out of place without it.

Removing the TARGET_MMA requirement is the correct thing to do no matter
what: this is not an MMA insn at all after all, and neither is it only
useful if MMA is enabled, etc.

Is the later "unreachable" ever useful?  If not, you could just fall
through after that "if (TARGET_MMA)" thing.  Worst that will happen you
get an OO move in the insn stream that we will error on later :-)

The patch is okay for trunk with the indentation fixed.  Thanks!


Segher