rs6000: Adjust mov optabs for opaque modes [PR103353]

Hi,

As PR103353 shows, we may want to continue to expand a MMA built-in
function like a normal 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 this patch is to allow the mov pattern to be generated when we are
expanding to RTL and have seen errors even without MMA supported, it's
expected that the generated pattern would not cause further ICEs as the
compilation would stop soon after expanding.

Bootstrapped and regtested on powerpc64-linux-gnu P8 and
powerpc64le-linux-gnu P9 and P10.

Is it ok for trunk?

BR,
Kewen
------

	PR target/103353

gcc/ChangeLog:

	* config/rs6000/mma.md (define_expand movoo): Move TARGET_MMA condition
	check to preparation statements and add handlings for !TARGET_MMA.
	(define_expand movxo): Likewise.
---
 gcc/config/rs6000/mma.md | 42 ++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

Hi,

Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591150.html

BR,
Kewen

on 2022/3/3 4:38 PM, Kewen.Lin via Gcc-patches wrote:
> Hi,
> 
> As PR103353 shows, we may want to continue to expand a MMA built-in
> function like a normal 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 this patch is to allow the mov pattern to be generated when we are
> expanding to RTL and have seen errors even without MMA supported, it's
> expected that the generated pattern would not cause further ICEs as the
> compilation would stop soon after expanding.
> 
> Bootstrapped and regtested on powerpc64-linux-gnu P8 and
> powerpc64le-linux-gnu P9 and P10.
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> ------
> 
> 	PR target/103353
> 
> gcc/ChangeLog:
> 
> 	* config/rs6000/mma.md (define_expand movoo): Move TARGET_MMA condition
> 	check to preparation statements and add handlings for !TARGET_MMA.
> 	(define_expand movxo): Likewise.

On Thu, 2022-03-03 at 16:38 +0800, Kewen.Lin via Gcc-patches wrote:
> Hi,
> 

Hi

> As PR103353 shows, we may want to continue to expand a MMA built-in
> function like a normal 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 this patch is to allow the mov pattern to be generated when we are
> expanding to RTL and have seen errors even without MMA supported, it's
> expected that the generated pattern would not cause further ICEs as the
> compilation would stop soon after expanding.

Is there a testcase, new or existing, that illustrates this error path?

> 
> Bootstrapped and regtested on powerpc64-linux-gnu P8 and
> powerpc64le-linux-gnu P9 and P10.
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> ------
> 
> 	PR target/103353
> 
> gcc/ChangeLog:
> 
> 	* config/rs6000/mma.md (define_expand movoo): Move TARGET_MMA condition
> 	check to preparation statements and add handlings for !TARGET_MMA.
> 	(define_expand movxo): Likewise.

> > ---
> >  gcc/config/rs6000/mma.md | 42 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md
> > index 907c9d6d516..f76a87b4a21 100644
> > --- a/gcc/config/rs6000/mma.md
> > +++ b/gcc/config/rs6000/mma.md
> > @@ -268,10 +268,25 @@ (define_int_attr avvi4i4i4	[(UNSPEC_MMA_PMXVI8GER4PP	"pmxvi8ger4pp")
> >  (define_expand "movoo"
> >    [(set (match_operand:OO 0 "nonimmediate_operand")
> >  	(match_operand:OO 1 "input_operand"))]
> > -  "TARGET_MMA"
> > +  ""
> >  {
> > -  rs6000_emit_move (operands[0], operands[1], OOmode);
> > -  DONE;
> > +  if (TARGET_MMA) {
> > +    rs6000_emit_move (operands[0], operands[1], OOmode);
> > +    DONE;
> > +  }
> > +  /* Opaque modes are only expected to be available when MMA is supported,
> > +     but PR103353 shows we may want to continue to expand a MMA built-in
> > +     function like a normal function, even if we have already emitted
> > +     error messages about some missing required conditions.

perhaps drop "like a normal function".  


> > +     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, even
> > +     though there is no MMA support.  It would not cause further ICEs as
> > +     the compilation would stop soon after expanding.  */

Testcase would be particularly helpful to illustrate this, i think.  

TH
anks,
-Will

> > +  else if (currently_expanding_to_rtl && seen_error ())
> > +    ;
> > +  else
> > +    gcc_unreachable ();
> >  })
> >  
> >  (define_insn_and_split "*movoo"
> > @@ -300,10 +315,25 @@ (define_insn_and_split "*movoo"
> >  (define_expand "movxo"
> >    [(set (match_operand:XO 0 "nonimmediate_operand")
> >  	(match_operand:XO 1 "input_operand"))]
> > -  "TARGET_MMA"
> > +  ""
> >  {
> > -  rs6000_emit_move (operands[0], operands[1], XOmode);
> > -  DONE;
> > +  if (TARGET_MMA) {
> > +    rs6000_emit_move (operands[0], operands[1], XOmode);
> > +    DONE;
> > +  }
> > +  /* Opaque modes are only expected to be available when MMA is supported,
> > +     but PR103353 shows we may want to continue to expand a MMA built-in
> > +     function like a normal 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, even
> > +     though there is no MMA support.  It would not cause further ICEs as
> > +     the compilation would stop soon after expanding.  */
> > +  else if (currently_expanding_to_rtl && seen_error ())
> > +    ;
> > +  else
> > +    gcc_unreachable ();
> >  })
> >  
> >  (define_insn_and_split "*movxo"
> > -- 
> > 2.25.1
> >

On 4/1/22 3:50 PM, will schmidt wrote:
> Is there a testcase, new or existing, that illustrates this error path?

Well, the already existsing test case pr101849.c is where the issue was seen,
but only when compiled by hand outside of the test harness and using only the
-maltivec option and not the -mcpu=power10 that the test case uses.

That said, I agree, pr101849.c should probably be copied into another test case
file (pr103353.c) that uses options that trigger the issue so we can be sure the
fix won't regress.

Peter

on 2022/4/2 5:52 AM, Peter Bergner wrote:
> On 4/1/22 3:50 PM, will schmidt wrote:
>> Is there a testcase, new or existing, that illustrates this error path?
> 
> Well, the already existsing test case pr101849.c is where the issue was seen,
> but only when compiled by hand outside of the test harness and using only the
> -maltivec option and not the -mcpu=power10 that the test case uses.
> 
> That said, I agree, pr101849.c should probably be copied into another test case
> file (pr103353.c) that uses options that trigger the issue so we can be sure the
> fix won't regress.
> 
> Peter
> 
> 

Thanks for the comments, Will and Peter!

I thought of adding one case before but gave up as the case still emits error messages
which can be fragile and are not test points and I didn't find any similar test cases
for similar scenarios in powerpc test suite.  Your suggestions make me consider it
seriously again.  The patch v2 was just posted with one test case:

  https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592916.html.

Hope it looks better to you, thanks again!

BR,
Kewen