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

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Segher,

Thanks for the review!

on 2022/8/16 05:30, Segher Boessenkool wrote:
> 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).
> 

Got it!

>> 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;
>     }
> 

Good catch, 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.
> 

Done.

> 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.

Yeah, it needs one subsequent patch as we agree on.

> 
> 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 :-)

Yeah, I agree that "just FAIL" works for this issue, but for now without
the valid condition in the context we only expect this expansion to just
perform for bif further expansion (knowing it's taken as bad), the
"unreachable" is meant to punt the other unexpected cases.  :)

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

Indentation and comments problems addressed, committed in r13-2062.

Thanks again!

BR,
Kewen