Message ID | 41da7001-549d-c7ae-fa6b-534a8faf673e@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 2883F3834E49 for <patchwork@sourceware.org>; Thu, 26 May 2022 07:36:21 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2883F3834E49 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1653550581; bh=rDcFWLta2hfKIGGdOAs6LpGTGXopLM2WxAZnpGHaS5Y=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=ZIRY+eKfUjqJcLHNn74pZdEQepqQCdHQtO0zpZMpIKnG2FkOWQXZHgsOzpdA/NFgT ANFdrYIrWjSMNircA2yuNNckGrmCwIlh+hVBAYzCjI9Y6Js/6rCfU5FE4xw2/WMXVk sOsMc0txyn9TogrCNuj+GbHvlVc5m5QtUtP8WqSs= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 26F09385E45D for <gcc-patches@gcc.gnu.org>; Thu, 26 May 2022 07:35:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 26F09385E45D Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 24Q46L1w014631; Thu, 26 May 2022 07:35:49 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3ga1qrbhvm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 26 May 2022 07:35:49 +0000 Received: from m0098409.ppops.net (m0098409.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 24Q7ZaCx028045; Thu, 26 May 2022 07:35:49 GMT Received: from ppma06ams.nl.ibm.com (66.31.33a9.ip4.static.sl-reverse.com [169.51.49.102]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3ga1qrbhv4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 26 May 2022 07:35:48 +0000 Received: from pps.filterd (ppma06ams.nl.ibm.com [127.0.0.1]) by ppma06ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 24Q7IT9N019808; Thu, 26 May 2022 07:35:46 GMT Received: from b06avi18878370.portsmouth.uk.ibm.com (b06avi18878370.portsmouth.uk.ibm.com [9.149.26.194]) by ppma06ams.nl.ibm.com with ESMTP id 3g93uwa7ad-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 26 May 2022 07:35:46 +0000 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06avi18878370.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 24Q7YogS32833904 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 26 May 2022 07:34:50 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 69D6242041; Thu, 26 May 2022 07:35:43 +0000 (GMT) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8278642042; Thu, 26 May 2022 07:35:41 +0000 (GMT) Received: from [9.200.42.164] (unknown [9.200.42.164]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 26 May 2022 07:35:41 +0000 (GMT) Message-ID: <41da7001-549d-c7ae-fa6b-534a8faf673e@linux.ibm.com> Date: Thu, 26 May 2022 15:35:39 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Content-Language: en-US To: gcc-patches <gcc-patches@gcc.gnu.org> Subject: [PATCH v2, rs6000] Fix ICE on expand bcd<bcd_add_sub>_<code>_<mode> [PR100736] Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: jfAAYc8Dz3B7oFLnomzEk2NbcAa1SDHg X-Proofpoint-GUID: 4Iy1qkQIzMo5o1cTM-LYXhyydvQd3eoZ 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-26_02,2022-05-25_02,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 phishscore=0 mlxscore=0 malwarescore=0 suspectscore=0 priorityscore=1501 bulkscore=0 mlxlogscore=999 lowpriorityscore=0 impostorscore=0 spamscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000 definitions=main-2205260037 X-Spam-Status: No, score=-12.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: HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: HAO CHEN GUI <guihaoc@linux.ibm.com> Cc: Peter Bergner <bergner@linux.ibm.com>, David <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 |
[v2,rs6000] Fix ICE on expand bcd<bcd_add_sub>_<code>_<mode> [PR100736]
|
|
Commit Message
HAO CHEN GUI
May 26, 2022, 7:35 a.m. UTC
Hi, This patch fixes the ICE reported in PR100736. It removes the condition check of finite math only flag not setting in "*<code><mode>_cc" pattern. With or without this flag, we still can use "cror" to check if either two bits of CC is set or not for "fp_two" codes. We don't need a reverse comparison (implemented by crnot) here when the finite math flag is set, as the latency of "cror" and "crnor" are the same. Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk? Any recommendations? Thanks a lot. ChangeLog 2022-05-26 Haochen Gui <guihaoc@linux.ibm.com> gcc/ * config/rs6000/rs6000.md (*<code><mode>_cc): Remove condition of finite math only flag not setting. gcc/testsuite/ * gcc.target/powerpc/pr100736.c: New. patch.diff
Comments
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595661.html Thanks. On 26/5/2022 下午 3:35, HAO CHEN GUI wrote: > Hi, > This patch fixes the ICE reported in PR100736. It removes the condition > check of finite math only flag not setting in "*<code><mode>_cc" pattern. > With or without this flag, we still can use "cror" to check if either > two bits of CC is set or not for "fp_two" codes. We don't need a reverse > comparison (implemented by crnot) here when the finite math flag is set, > as the latency of "cror" and "crnor" are the same. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > ChangeLog > 2022-05-26 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > * config/rs6000/rs6000.md (*<code><mode>_cc): Remove condition of > finite math only flag not setting. > > gcc/testsuite/ > * gcc.target/powerpc/pr100736.c: New. > > > patch.diff > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index fdfbc6566a5..a6f9cbc9b8b 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -12995,9 +12995,9 @@ (define_insn_and_split "*<code><mode>_cc" > [(set (match_operand:GPR 0 "gpc_reg_operand" "=r") > (fp_two:GPR (match_operand:CCFP 1 "cc_reg_operand" "y") > (const_int 0)))] > - "!flag_finite_math_only" > + "" > "#" > - "&& 1" > + "" > [(pc)] > { > rtx cc = rs6000_emit_fp_cror (<CODE>, <MODE>mode, operands[1]); > diff --git a/gcc/testsuite/gcc.target/powerpc/pr100736.c b/gcc/testsuite/gcc.target/powerpc/pr100736.c > new file mode 100644 > index 00000000000..32cb6df6cd9 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr100736.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > +/* { dg-options "-mdejagnu-cpu=power8 -O2 -ffinite-math-only" } */ > + > +typedef __attribute__ ((altivec (vector__))) unsigned char v; > + > +int foo (v a, v b) > +{ > + return __builtin_vec_bcdsub_ge (a, b, 0); > +} > + > +/* { dg-final { scan-assembler {\mcror\M} } } */ >
Hi Haochen, on 2022/5/26 15:35, HAO CHEN GUI wrote: > Hi, > This patch fixes the ICE reported in PR100736. It removes the condition > check of finite math only flag not setting in "*<code><mode>_cc" pattern. > With or without this flag, we still can use "cror" to check if either > two bits of CC is set or not for "fp_two" codes. We don't need a reverse > comparison (implemented by crnot) here when the finite math flag is set, > as the latency of "cror" and "crnor" are the same. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > ChangeLog > 2022-05-26 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > * config/rs6000/rs6000.md (*<code><mode>_cc): Remove condition of > finite math only flag not setting. > > gcc/testsuite/ > * gcc.target/powerpc/pr100736.c: New. > > > patch.diff > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index fdfbc6566a5..a6f9cbc9b8b 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -12995,9 +12995,9 @@ (define_insn_and_split "*<code><mode>_cc" > [(set (match_operand:GPR 0 "gpc_reg_operand" "=r") > (fp_two:GPR (match_operand:CCFP 1 "cc_reg_operand" "y") > (const_int 0)))] > - "!flag_finite_math_only" > + "" > "#" > - "&& 1" > + "" Segher added this hunk, not sure if he prefer to keep the condition unchanged and update the expansion side, looking forward to his comments. :) > [(pc)] > { > rtx cc = rs6000_emit_fp_cror (<CODE>, <MODE>mode, operands[1]); > diff --git a/gcc/testsuite/gcc.target/powerpc/pr100736.c b/gcc/testsuite/gcc.target/powerpc/pr100736.c > new file mode 100644 > index 00000000000..32cb6df6cd9 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr100736.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > +/* { dg-options "-mdejagnu-cpu=power8 -O2 -ffinite-math-only" } */ > + > +typedef __attribute__ ((altivec (vector__))) unsigned char v; > + > +int foo (v a, v b) > +{ > + return __builtin_vec_bcdsub_ge (a, b, 0); > +} > + > +/* { dg-final { scan-assembler {\mcror\M} } } */ > The case of PR100736 fails with ICE as reported, maybe we can remove this dg-final check, since as you noted in the description above either "cror" or "crnor" are acceptable, this extra check could probably make this case fragile. BR, Kewen
Hi! On Mon, May 30, 2022 at 06:12:26PM +0800, Kewen.Lin wrote: > on 2022/5/26 15:35, HAO CHEN GUI wrote: > > This patch fixes the ICE reported in PR100736. It removes the condition > > check of finite math only flag not setting in "*<code><mode>_cc" pattern. > > With or without this flag, we still can use "cror" to check if either > > two bits of CC is set or not for "fp_two" codes. We don't need a reverse > > comparison (implemented by crnot) here when the finite math flag is set, > > as the latency of "cror" and "crnor" are the same. > > --- a/gcc/config/rs6000/rs6000.md > > +++ b/gcc/config/rs6000/rs6000.md > > @@ -12995,9 +12995,9 @@ (define_insn_and_split "*<code><mode>_cc" > > [(set (match_operand:GPR 0 "gpc_reg_operand" "=r") > > (fp_two:GPR (match_operand:CCFP 1 "cc_reg_operand" "y") > > (const_int 0)))] > > - "!flag_finite_math_only" > > + "" > > "#" > > - "&& 1" > > + "" > > Segher added this hunk, not sure if he prefer to keep the condition unchanged > and update the expansion side, looking forward to his comments. :) It's not clear to me how this can ever happen without finite_math_only? The patch is safe, sure, but it may the real problem is elsewhere. > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/powerpc/pr100736.c > > @@ -0,0 +1,12 @@ > > +/* { dg-do compile } */ > > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > > +/* { dg-options "-mdejagnu-cpu=power8 -O2 -ffinite-math-only" } */ The usual flag to use would be -ffast-math :-) > > +/* { dg-final { scan-assembler {\mcror\M} } } */ > > The case of PR100736 fails with ICE as reported, maybe we can remove this dg-final check, > since as you noted in the description above either "cror" or "crnor" are acceptable, > this extra check could probably make this case fragile. Check for \mcrn?or\M then? But, is crnor something we want here ever? The reason we do not have cror for finte-math-only is that comparisons can only (validly :-) ) return LT, GT, or EQ then, and we can branch on that without twiddling CRF bits first. Is this not true for BCD compares, is that what the problem is? Or, is our builtin expansion returning something invalid? Or something else :-) Segher
Hi! On Tue, May 31, 2022 at 06:56:00PM -0500, Segher Boessenkool wrote: > It's not clear to me how this can ever happen without finite_math_only? > The patch is safe, sure, but it may the real problem is elsewhere. So, it is incorrect the RTL for our bcd{add,sub} insns uses CCFP at all. CCFP stands for the result of a 4-way comparison, regular float comparison: lt gt eq un. But bcdadd does not have an unordered at all. Instead, it has the result of a 3-way comparison (lt gt eq), and bit 3 is set if an overflow happened -- but still exactly one of bits 0..2 is set then! (If one of the inputs is an invalid number it sets bits 0..3 to 0001 though.) So it would be much more correct and sensible to use regular integer comparison results here, so, CC. Does that fix the problem? Segher
Segher, Does BCD comparison return false when either operand is invalid coding? If yes, the result could be 3-way. We can check gt and eq bits for ge. We still can't use crnot to only check lt bit as there could be invalid coding. Also, do you think finite-math-only excludes invalid coding? Seems GCC doesn't clear define it. Thanks. Gui Haochen On 2/6/2022 上午 6:05, Segher Boessenkool wrote: > Hi! > > On Tue, May 31, 2022 at 06:56:00PM -0500, Segher Boessenkool wrote: >> It's not clear to me how this can ever happen without finite_math_only? >> The patch is safe, sure, but it may the real problem is elsewhere. > > So, it is incorrect the RTL for our bcd{add,sub} insns uses CCFP at all. > > CCFP stands for the result of a 4-way comparison, regular float > comparison: lt gt eq un. But bcdadd does not have an unordered at all. > Instead, it has the result of a 3-way comparison (lt gt eq), and bit 3 > is set if an overflow happened -- but still exactly one of bits 0..2 is > set then! (If one of the inputs is an invalid number it sets bits 0..3 > to 0001 though.) > > So it would be much more correct and sensible to use regular integer > comparison results here, so, CC. > > Does that fix the problem? > > > Segher
Hi! On Thu, Jun 02, 2022 at 01:30:04PM +0800, HAO CHEN GUI wrote: > Segher, > Does BCD comparison return false when either operand is invalid coding? It sets all of LT, GT, and EQ to 0 (it normally sets exactly one of them to 1). It sets bit 3 (the "SO" bit usually) to 1. That is what the machine insns do. What the builtins do is undefined as far as I know? If So we can do whatever is most convenient, so, not handle it specifically at all, just go with what falls out. > If yes, the result could be 3-way. We can check gt and eq bits for ge. You can check the LT bit, instead: it is only one branch insn, and also only one setbc[r] insn (it can be slightly more expensive if you can use only older insns). > We still can't use crnot to only check lt bit as there could be invalid > coding. > Also, do you think finite-math-only excludes invalid coding? Seems GCC > doesn't clear define it. This is not floating-point code at all, it should not be influenced at all by finite-math-only! Segher
On 2/6/2022 下午 5:04, Segher Boessenkool wrote: > Hi! > > On Thu, Jun 02, 2022 at 01:30:04PM +0800, HAO CHEN GUI wrote: >> Segher, >> Does BCD comparison return false when either operand is invalid coding? > > It sets all of LT, GT, and EQ to 0 (it normally sets exactly one of them > to 1). It sets bit 3 (the "SO" bit usually) to 1. > > That is what the machine insns do. What the builtins do is undefined as > far as I know? If So we can do whatever is most convenient, so, not > handle it specifically at all, just go with what falls out. We defined the following unordered BCD builtins in rs6000-builtin.def. They check the bit 3 for overflow. const signed int __builtin_bcdadd_ov_v1ti (vsq, vsq, const int<1>); BCDADD_OV_V1TI bcdadd_unordered_v1ti {} const signed int __builtin_bcdadd_ov_v16qi (vsc, vsc, const int<1>); BCDADD_OV_V16QI bcdadd_unordered_v16qi {} Also Xlc defines three BCD builtins for overflow and invalid coding. https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.1?topic=functions-bcd-test-add-subtract-overflow Shall GCC keep up with Xlc? Please advise. Thanks Gui Haochen > >> If yes, the result could be 3-way. We can check gt and eq bits for ge. > > You can check the LT bit, instead: it is only one branch insn, and also > only one setbc[r] insn (it can be slightly more expensive if you can use > only older insns). > >> We still can't use crnot to only check lt bit as there could be invalid >> coding. >> Also, do you think finite-math-only excludes invalid coding? Seems GCC >> doesn't clear define it. > > This is not floating-point code at all, it should not be influenced at > all by finite-math-only! > > > Segher
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index fdfbc6566a5..a6f9cbc9b8b 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -12995,9 +12995,9 @@ (define_insn_and_split "*<code><mode>_cc" [(set (match_operand:GPR 0 "gpc_reg_operand" "=r") (fp_two:GPR (match_operand:CCFP 1 "cc_reg_operand" "y") (const_int 0)))] - "!flag_finite_math_only" + "" "#" - "&& 1" + "" [(pc)] { rtx cc = rs6000_emit_fp_cror (<CODE>, <MODE>mode, operands[1]); diff --git a/gcc/testsuite/gcc.target/powerpc/pr100736.c b/gcc/testsuite/gcc.target/powerpc/pr100736.c new file mode 100644 index 00000000000..32cb6df6cd9 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr100736.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target powerpc_p8vector_ok } */ +/* { dg-options "-mdejagnu-cpu=power8 -O2 -ffinite-math-only" } */ + +typedef __attribute__ ((altivec (vector__))) unsigned char v; + +int foo (v a, v b) +{ + return __builtin_vec_bcdsub_ge (a, b, 0); +} + +/* { dg-final { scan-assembler {\mcror\M} } } */