Message ID | 09c34b29-feea-d26e-2c4f-5e096ab286bc@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 673F5383E83D for <patchwork@sourceware.org>; Wed, 18 May 2022 14:09:11 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 673F5383E83D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1652882951; bh=MezNBufBO6l9en0bDHNycOyQJE2BrhqP3SwHRFXDs4E=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=ZWibPL0i5i0FzDVrKjLkR/Y1bprSHf32NtFMlPPv4UiXmaxI0PPkhF+oQ4eIzcJGE h73qbqDRJQD9+1oEFkrKZJD1e1D6wUlfaw2ihP3VyB0RVtC2UTCyQpNKrl76bG7mMb w/xxeuYVvnfTPEqvrAzm7ADi4IYfdwc7CcJmZONw= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id ACC503846030 for <gcc-patches@gcc.gnu.org>; Wed, 18 May 2022 14:07:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org ACC503846030 Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 24IDplSe011238; Wed, 18 May 2022 14:07:57 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3g5228ggnj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 18 May 2022 14:07:57 +0000 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 24IE0D6d024359; Wed, 18 May 2022 14:07:56 GMT Received: from ppma03fra.de.ibm.com (6b.4a.5195.ip4.static.sl-reverse.com [149.81.74.107]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3g5228ggmc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 18 May 2022 14:07:56 +0000 Received: from pps.filterd (ppma03fra.de.ibm.com [127.0.0.1]) by ppma03fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 24IE2UOq013106; Wed, 18 May 2022 14:07:55 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma03fra.de.ibm.com with ESMTP id 3g2428vq7j-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 18 May 2022 14:07:54 +0000 Received: from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com [9.149.105.59]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 24IDs0gg55247260 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 18 May 2022 13:54:00 GMT Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 6175CA404D; Wed, 18 May 2022 14:07:52 +0000 (GMT) Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 40372A4040; Wed, 18 May 2022 14:07:50 +0000 (GMT) Received: from [9.197.252.204] (unknown [9.197.252.204]) by d06av23.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 18 May 2022 14:07:49 +0000 (GMT) Message-ID: <09c34b29-feea-d26e-2c4f-5e096ab286bc@linux.ibm.com> Date: Wed, 18 May 2022 22:07:48 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Content-Language: en-US To: GCC Patches <gcc-patches@gcc.gnu.org> Subject: [PATCH v3] rs6000: Adjust mov optabs for opaque modes [PR103353] Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: 7ztI-Tim5-wgyC048iVFtvMb_4kGTbXU X-Proofpoint-GUID: q13FHha0ggYdzcQwLn42_mPZmMWXb54t Content-Transfer-Encoding: 7bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.874,Hydra:6.0.486,FMLib:17.11.64.514 definitions=2022-05-18_05,2022-05-17_02,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 impostorscore=0 mlxscore=0 spamscore=0 adultscore=0 phishscore=0 suspectscore=0 bulkscore=0 mlxlogscore=999 malwarescore=0 lowpriorityscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000 definitions=main-2205180083 X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: "Kewen.Lin via Gcc-patches" <gcc-patches@gcc.gnu.org> Reply-To: "Kewen.Lin" <linkw@linux.ibm.com> Cc: Peter Bergner <bergner@linux.ibm.com>, David Edelsohn <dje.gcc@gmail.com>, Segher Boessenkool <segher@kernel.crashing.org> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[v3] rs6000: Adjust mov optabs for opaque modes [PR103353]
|
|
Commit Message
Kewen.Lin
May 18, 2022, 2:07 p.m. UTC
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. v3: Update test case with dg-excess-errors. v2: Polish some comments and add one test case as Will and Peter suggested. https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592916.html v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591150.html 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/testsuite/ChangeLog: * gcc.target/powerpc/pr103353.c: New test. --- gcc/config/rs6000/mma.md | 42 ++++++++++++++++++--- gcc/testsuite/gcc.target/powerpc/pr103353.c | 22 +++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103353.c
Comments
Hi, Gentle ping https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595209.html BR, Kewen on 2022/5/18 22:07, 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. > > v3: Update test case with dg-excess-errors. > > v2: Polish some comments and add one test case as Will and Peter suggested. > https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592916.html > > v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591150.html > > 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/testsuite/ChangeLog: > > * gcc.target/powerpc/pr103353.c: New test. > --- > gcc/config/rs6000/mma.md | 42 ++++++++++++++++++--- > gcc/testsuite/gcc.target/powerpc/pr103353.c | 22 +++++++++++ > 2 files changed, 58 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103353.c > > diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md > index 907c9d6d516..746a77a0957 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, 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 "*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, even if we have already emitted error messages about some > + missing required conditions. As shown in that PR, without one > + explicit mov optab on XOmode 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" > diff --git a/gcc/testsuite/gcc.target/powerpc/pr103353.c b/gcc/testsuite/gcc.target/powerpc/pr103353.c > new file mode 100644 > index 00000000000..5d519fb1b7b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr103353.c > @@ -0,0 +1,22 @@ > +/* { dg-require-effective-target powerpc_altivec_ok } */ > +/* If the default cpu type is power10 or later, MMA is enabled by default. > + To keep the test point available all the time, this case specifies > + -mdejagnu-cpu=power6 to make it be tested without MMA. */ > +/* { dg-options "-maltivec -mdejagnu-cpu=power6" } */ > + > +/* Verify there is no ICE and don't check the error messages on MMA > + requirement since they could be fragile and are not test points > + of this case. */ > +/* { dg-excess-errors "pr103353" } */ > + > +void > +foo (__vector_pair *dst, double *x) > +{ > + dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); > +} > + > +void > +bar (__vector_pair *src, double *x) > +{ > + __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x); > +}
Hi, Gentle ping https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595209.html BR, Kewen >> 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. >> >> v3: Update test case with dg-excess-errors. >> >> v2: Polish some comments and add one test case as Will and Peter suggested. >> https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592916.html >> >> v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591150.html >> >> 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/testsuite/ChangeLog: >> >> * gcc.target/powerpc/pr103353.c: New test. >> --- >> gcc/config/rs6000/mma.md | 42 ++++++++++++++++++--- >> gcc/testsuite/gcc.target/powerpc/pr103353.c | 22 +++++++++++ >> 2 files changed, 58 insertions(+), 6 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103353.c >> >> diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md >> index 907c9d6d516..746a77a0957 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, 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 "*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, even if we have already emitted error messages about some >> + missing required conditions. As shown in that PR, without one >> + explicit mov optab on XOmode 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" >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103353.c b/gcc/testsuite/gcc.target/powerpc/pr103353.c >> new file mode 100644 >> index 00000000000..5d519fb1b7b >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr103353.c >> @@ -0,0 +1,22 @@ >> +/* { dg-require-effective-target powerpc_altivec_ok } */ >> +/* If the default cpu type is power10 or later, MMA is enabled by default. >> + To keep the test point available all the time, this case specifies >> + -mdejagnu-cpu=power6 to make it be tested without MMA. */ >> +/* { dg-options "-maltivec -mdejagnu-cpu=power6" } */ >> + >> +/* Verify there is no ICE and don't check the error messages on MMA >> + requirement since they could be fragile and are not test points >> + of this case. */ >> +/* { dg-excess-errors "pr103353" } */ >> + >> +void >> +foo (__vector_pair *dst, double *x) >> +{ >> + dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); >> +} >> + >> +void >> +bar (__vector_pair *src, double *x) >> +{ >> + __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x); >> +} >
Hi! On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote: > 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. First off: lxvp is a VSX insn, not an MMA insn. So please don't call it that -- this confusion is what presumably caused the problem here, so it would be good to root it out :-) > + /* Opaque modes are only expected to be available when MMA is supported, Why do people expect that? It is completely wrong. The name "opaque" itself already says this is not just for MMA, but perhaps more importantly, it is a basic VSX insn, doesn't touch any MMA resources, and is useful in other contexts as well. So this needs some bigger surgery. Segher
Hi Segher, Thanks for the comments. on 2022/6/24 03:06, Segher Boessenkool wrote: > Hi! > > On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote: >> 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. > > First off: lxvp is a VSX insn, not an MMA insn. So please don't call it > that -- this confusion is what presumably caused the problem here, so it > would be good to root it out :-) > I guess the "it" in "don't call it call" is for "MMA built-in function"? It comes from the current code: ; mma Needs special handling for MMA ; quad MMA instruction using a register quad as an input operand ; pair MMA instruction using a register pair as an input operand v256 __builtin_vsx_lxvp (unsigned long, const v256 *); LXVP nothing {mma} void __builtin_vsx_stxvp (v256, unsigned long, const v256 *); STXVP nothing {mma,pair} ... >> + /* Opaque modes are only expected to be available when MMA is supported, > > Why do people expect that? It is completely wrong. The name "opaque" > itself already says this is not just for MMA, but perhaps more > importantly, it is a basic VSX insn, doesn't touch any MMA resources, > and is useful in other contexts as well. > ... The above statements are also based on current code, for now, the related things like built-in functions, mov optab, hard_regno_ok etc. for these two modes are guarded by TARGET_MMA. I think I get your points here, you want to separate these opaque modes from MMA since the underlying lxvp/stxvp are not MMA specific, so those related things (bifs, mov optabs etc.) are not necessarily guarded under MMA. > So this needs some bigger surgery. Yes, Peter may have more comments on this. BR, Kewen
Hi! On Fri, Jun 24, 2022 at 09:03:59AM +0800, Kewen.Lin wrote: > on 2022/6/24 03:06, Segher Boessenkool wrote: > > On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote: > >> 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. > > > > First off: lxvp is a VSX insn, not an MMA insn. So please don't call it > > that -- this confusion is what presumably caused the problem here, so it > > would be good to root it out :-) > > I guess the "it" in "don't call it call" is for "MMA built-in function"? > It comes from the current code: Your proposed commit message says "MMA built-in function". It is not an MMA builtin, or rather, it should not be. > >> + /* Opaque modes are only expected to be available when MMA is supported, > > > > Why do people expect that? It is completely wrong. The name "opaque" > > itself already says this is not just for MMA, but perhaps more > > importantly, it is a basic VSX insn, doesn't touch any MMA resources, > > and is useful in other contexts as well. > > ... The above statements are also based on current code, for now, the > related things like built-in functions, mov optab, hard_regno_ok etc. > for these two modes are guarded by TARGET_MMA. Opaque modes are a generic thing, not an rs6000 thing. It is important not to conflate completely different things that just happened to coincide some months ago (but not anymore right now even!) > I think I get your points here, you want to separate these opaque > modes from MMA since the underlying lxvp/stxvp are not MMA specific, > so those related things (bifs, mov optabs etc.) are not necessarily > guarded under MMA. Yup. This can take some time of course, but in the mean time we should stop pretending the status quo is correct. > > So this needs some bigger surgery. > > Yes, Peter may have more comments on this. Yes. Can you do a patch that just fixes this PR103353, without adding more misleading comments? :-) Segher
Hi Segher! on 2022/6/25 00:49, Segher Boessenkool wrote: > Hi! > > On Fri, Jun 24, 2022 at 09:03:59AM +0800, Kewen.Lin wrote: >> on 2022/6/24 03:06, Segher Boessenkool wrote: >>> On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote: >>>> 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. >>> >>> First off: lxvp is a VSX insn, not an MMA insn. So please don't call it >>> that -- this confusion is what presumably caused the problem here, so it >>> would be good to root it out :-) >> >> I guess the "it" in "don't call it call" is for "MMA built-in function"? >> It comes from the current code: > > Your proposed commit message says "MMA built-in function". It is not > an MMA builtin, or rather, it should not be. > >>>> + /* Opaque modes are only expected to be available when MMA is supported, >>> >>> Why do people expect that? It is completely wrong. The name "opaque" >>> itself already says this is not just for MMA, but perhaps more >>> importantly, it is a basic VSX insn, doesn't touch any MMA resources, >>> and is useful in other contexts as well. >> >> ... The above statements are also based on current code, for now, the >> related things like built-in functions, mov optab, hard_regno_ok etc. >> for these two modes are guarded by TARGET_MMA. > > Opaque modes are a generic thing, not an rs6000 thing. It is important > not to conflate completely different things that just happened to > coincide some months ago (but not anymore right now even!) > >> I think I get your points here, you want to separate these opaque >> modes from MMA since the underlying lxvp/stxvp are not MMA specific, >> so those related things (bifs, mov optabs etc.) are not necessarily >> guarded under MMA. > > Yup. This can take some time of course, but in the mean time we should > stop pretending the status quo is correct. > >>> So this needs some bigger surgery. >> >> Yes, Peter may have more comments on this. > > Yes. Can you do a patch that just fixes this PR103353, without adding > more misleading comments? :-) > 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? BR, Kewen From ee49cd14b69aaa373b0aca71c4560944a0b43fbc Mon Sep 17 00:00:00 2001 From: "Kewen.Lin" <linkw@gcc.gnu.org> Date: Mon, 27 Jun 2022 10:42:37 +0800 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. So this patch is to allow the mov pattern to be generated during expanding phase if compiler has already seen errors. 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/testsuite/ChangeLog: * gcc.target/powerpc/pr103353.c: New test. --- gcc/config/rs6000/mma.md | 35 +++++++++++++++++---- gcc/testsuite/gcc.target/powerpc/pr103353.c | 22 +++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103353.c diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md index a183b6a168a..a9cf59d68b5 100644 --- a/gcc/config/rs6000/mma.md +++ b/gcc/config/rs6000/mma.md @@ -268,10 +268,23 @@ (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; + } + /* 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 ()) + ; + else + gcc_unreachable (); }) (define_insn_and_split "*movoo" @@ -300,10 +313,20 @@ (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; + } + /* 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. So do the same handlings for XOmode + as OOmode here. */ + else if (currently_expanding_to_rtl && seen_error ()) + ; + else + gcc_unreachable (); }) (define_insn_and_split "*movxo" diff --git a/gcc/testsuite/gcc.target/powerpc/pr103353.c b/gcc/testsuite/gcc.target/powerpc/pr103353.c new file mode 100644 index 00000000000..5d519fb1b7b --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr103353.c @@ -0,0 +1,22 @@ +/* { dg-require-effective-target powerpc_altivec_ok } */ +/* If the default cpu type is power10 or later, MMA is enabled by default. + To keep the test point available all the time, this case specifies + -mdejagnu-cpu=power6 to make it be tested without MMA. */ +/* { dg-options "-maltivec -mdejagnu-cpu=power6" } */ + +/* Verify there is no ICE and don't check the error messages on MMA + requirement since they could be fragile and are not test points + of this case. */ +/* { dg-excess-errors "pr103353" } */ + +void +foo (__vector_pair *dst, double *x) +{ + dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); +} + +void +bar (__vector_pair *src, double *x) +{ + __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x); +}
Hi, Gentle ping https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597286.html BR, Kewen on 2022/6/27 10:47, Kewen.Lin via Gcc-patches wrote: > Hi Segher! > > on 2022/6/25 00:49, Segher Boessenkool wrote: >> Hi! >> >> On Fri, Jun 24, 2022 at 09:03:59AM +0800, Kewen.Lin wrote: >>> on 2022/6/24 03:06, Segher Boessenkool wrote: >>>> On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote: >>>>> 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. >>>> >>>> First off: lxvp is a VSX insn, not an MMA insn. So please don't call it >>>> that -- this confusion is what presumably caused the problem here, so it >>>> would be good to root it out :-) >>> >>> I guess the "it" in "don't call it call" is for "MMA built-in function"? >>> It comes from the current code: >> >> Your proposed commit message says "MMA built-in function". It is not >> an MMA builtin, or rather, it should not be. >> >>>>> + /* Opaque modes are only expected to be available when MMA is supported, >>>> >>>> Why do people expect that? It is completely wrong. The name "opaque" >>>> itself already says this is not just for MMA, but perhaps more >>>> importantly, it is a basic VSX insn, doesn't touch any MMA resources, >>>> and is useful in other contexts as well. >>> >>> ... The above statements are also based on current code, for now, the >>> related things like built-in functions, mov optab, hard_regno_ok etc. >>> for these two modes are guarded by TARGET_MMA. >> >> Opaque modes are a generic thing, not an rs6000 thing. It is important >> not to conflate completely different things that just happened to >> coincide some months ago (but not anymore right now even!) >> >>> I think I get your points here, you want to separate these opaque >>> modes from MMA since the underlying lxvp/stxvp are not MMA specific, >>> so those related things (bifs, mov optabs etc.) are not necessarily >>> guarded under MMA. >> >> Yup. This can take some time of course, but in the mean time we should >> stop pretending the status quo is correct. >> >>>> So this needs some bigger surgery. >>> >>> Yes, Peter may have more comments on this. >> >> Yes. Can you do a patch that just fixes this PR103353, without adding >> more misleading comments? :-) >> > > 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? > > BR, > Kewen
Hi, Gentle ping https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597286.html BR, Kewen > > on 2022/6/27 10:47, Kewen.Lin via Gcc-patches wrote: >> Hi Segher! >> >> on 2022/6/25 00:49, Segher Boessenkool wrote: >>> Hi! >>> >>> On Fri, Jun 24, 2022 at 09:03:59AM +0800, Kewen.Lin wrote: >>>> on 2022/6/24 03:06, Segher Boessenkool wrote: >>>>> On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote: >>>>>> 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. >>>>> >>>>> First off: lxvp is a VSX insn, not an MMA insn. So please don't call it >>>>> that -- this confusion is what presumably caused the problem here, so it >>>>> would be good to root it out :-) >>>> >>>> I guess the "it" in "don't call it call" is for "MMA built-in function"? >>>> It comes from the current code: >>> >>> Your proposed commit message says "MMA built-in function". It is not >>> an MMA builtin, or rather, it should not be. >>> >>>>>> + /* Opaque modes are only expected to be available when MMA is supported, >>>>> >>>>> Why do people expect that? It is completely wrong. The name "opaque" >>>>> itself already says this is not just for MMA, but perhaps more >>>>> importantly, it is a basic VSX insn, doesn't touch any MMA resources, >>>>> and is useful in other contexts as well. >>>> >>>> ... The above statements are also based on current code, for now, the >>>> related things like built-in functions, mov optab, hard_regno_ok etc. >>>> for these two modes are guarded by TARGET_MMA. >>> >>> Opaque modes are a generic thing, not an rs6000 thing. It is important >>> not to conflate completely different things that just happened to >>> coincide some months ago (but not anymore right now even!) >>> >>>> I think I get your points here, you want to separate these opaque >>>> modes from MMA since the underlying lxvp/stxvp are not MMA specific, >>>> so those related things (bifs, mov optabs etc.) are not necessarily >>>> guarded under MMA. >>> >>> Yup. This can take some time of course, but in the mean time we should >>> stop pretending the status quo is correct. >>> >>>>> So this needs some bigger surgery. >>>> >>>> Yes, Peter may have more comments on this. >>> >>> Yes. Can you do a patch that just fixes this PR103353, without adding >>> more misleading comments? :-) >>> >> >> 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? >> >> BR, >> Kewen
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
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
diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md index 907c9d6d516..746a77a0957 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, 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 "*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, even if we have already emitted error messages about some + missing required conditions. As shown in that PR, without one + explicit mov optab on XOmode 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" diff --git a/gcc/testsuite/gcc.target/powerpc/pr103353.c b/gcc/testsuite/gcc.target/powerpc/pr103353.c new file mode 100644 index 00000000000..5d519fb1b7b --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr103353.c @@ -0,0 +1,22 @@ +/* { dg-require-effective-target powerpc_altivec_ok } */ +/* If the default cpu type is power10 or later, MMA is enabled by default. + To keep the test point available all the time, this case specifies + -mdejagnu-cpu=power6 to make it be tested without MMA. */ +/* { dg-options "-maltivec -mdejagnu-cpu=power6" } */ + +/* Verify there is no ICE and don't check the error messages on MMA + requirement since they could be fragile and are not test points + of this case. */ +/* { dg-excess-errors "pr103353" } */ + +void +foo (__vector_pair *dst, double *x) +{ + dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x); +} + +void +bar (__vector_pair *src, double *x) +{ + __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x); +}