Message ID | 585a2224-e076-9d26-921b-6db56f1606b9@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 58171385843D for <patchwork@sourceware.org>; Wed, 17 Nov 2021 20:59:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 58171385843D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1637182768; bh=fXo2ye1ZKVS+N2kPwhZimbVbiYp9w4YaPdpk6mLFUXg=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=T4OiEjaNcMyh6SaxDvh/gkxFE0IXNqLZ4Lb5x3lVZDAGpFje13n8G88sKGEiJob56 40EgToxEfdFomRo/zCtY9Tyvf/01rSLf1Ux2U0p2XtFfdFe5edQHf/AROuJa7J+NA2 YmiuvO130S1x5elWEHK0xTuguFfdjTAn+uUOgj5I= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 5C5773858402 for <gcc-patches@gcc.gnu.org>; Wed, 17 Nov 2021 20:58:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5C5773858402 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 1AHKfaaF030114; Wed, 17 Nov 2021 20:58:58 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3cd90arwh3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 17 Nov 2021 20:58:57 +0000 Received: from m0098420.ppops.net (m0098420.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 1AHKjEkL014791; Wed, 17 Nov 2021 20:58:57 GMT Received: from ppma05wdc.us.ibm.com (1b.90.2fa9.ip4.static.sl-reverse.com [169.47.144.27]) by mx0b-001b2d01.pphosted.com with ESMTP id 3cd90arwg9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 17 Nov 2021 20:58:57 +0000 Received: from pps.filterd (ppma05wdc.us.ibm.com [127.0.0.1]) by ppma05wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 1AHKwrqe013434; Wed, 17 Nov 2021 20:58:56 GMT Received: from b03cxnp08025.gho.boulder.ibm.com (b03cxnp08025.gho.boulder.ibm.com [9.17.130.17]) by ppma05wdc.us.ibm.com with ESMTP id 3ca50c5drv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 17 Nov 2021 20:58:56 +0000 Received: from b03ledav006.gho.boulder.ibm.com (b03ledav006.gho.boulder.ibm.com [9.17.130.237]) by b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 1AHKwthc44237220 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 17 Nov 2021 20:58:55 GMT Received: from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 69260C607C; Wed, 17 Nov 2021 20:58:55 +0000 (GMT) Received: from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 37AC1C606D; Wed, 17 Nov 2021 20:58:55 +0000 (GMT) Received: from [9.211.84.243] (unknown [9.211.84.243]) by b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP; Wed, 17 Nov 2021 20:58:55 +0000 (GMT) Message-ID: <585a2224-e076-9d26-921b-6db56f1606b9@linux.ibm.com> Date: Wed, 17 Nov 2021 14:58:54 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 To: GCC Patches <gcc-patches@gcc.gnu.org> Subject: [PATCH] rs6000: Builtins test changes for BFP scalar tests Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: hnEjLTzAzgq737hrk4ik8gDqgCfo6ZAW X-Proofpoint-ORIG-GUID: tQdHHeL3CFQUaY-Caf7XY0upEtRtSKdQ X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475 definitions=2021-11-17_07,2021-11-17_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 priorityscore=1501 lowpriorityscore=0 bulkscore=0 phishscore=0 spamscore=0 clxscore=1015 suspectscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2111170091 X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Bill Schmidt via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: wschmidt@linux.ibm.com Cc: 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 |
rs6000: Builtins test changes for BFP scalar tests
|
|
Commit Message
Li, Pan2 via Gcc-patches
Nov. 17, 2021, 8:58 p.m. UTC
Hi! This patch is broken out of the previous patch for all the builtins test suite adjustments. Here we have some slight changes in error messages due to how the internals have changed between the old and new builtins methods. For scalar-extract-exp-2.c we change: error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration' to: error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp' The new message provides more information. In both cases, it is less than ideal that we don't refer to scalar_extract_exp, which is referenced in the source line, but this is because scalar_extract_exp is #define'd to __builtin_vec_scalar_extract_exp, so it's unavoidable. Certainly this is no worse than before, and arguably better. The cases for: scalar-insert-exp-2.c scalar-insert-exp-5.c scalar-insert-exp-8.c are all similar. For scalar-extract-sig-2.c we again change: error: '__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration' to: error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig' Here it is clearer because there is no #define to muddy things up, and again the new message is arguably better than the old. For scalar-test-neg-{2,3,5}.c, we actually change the test case. This is because we deliberately removed some undocumented and pointless overloads, where each overload mapped to a single builtin. These were: __builtin_vec_scalar_test_neg_sp __builtin_vec_scalar_test_neg_dp __builtin_vec_scalar_test_neg_qp which are redundant with the "real" overload: __builtin_vec_scalar_test_neg The latter maps to three builtins of the appropriate type. The revised test case uses the "real" overload instead, and otherwise the changes to the error messages are the same as for all the other cases. 2021-11-17 Bill Schmidt <wschmidt@linux.ibm.com> gcc/testsuite/ * gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust error message. * gcc.target/powerpc/bfp/scalar-extract-sig-2.c: Likewise. * gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Likewise. * gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Likewise. * gcc.target/powerpc/bfp/scalar-insert-exp-8.c: Likewise. * gcc.target/powerpc/bfp/scalar-test-neg-2.c: Likewise. * gcc.target/powerpc/bfp/scalar-test-neg-3.c: Likewise. * gcc.target/powerpc/bfp/scalar-test-neg-5.c: Likewise. --- gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c | 2 +- gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-)
Comments
On Wed, Nov 17, 2021 at 02:58:54PM -0600, Bill Schmidt wrote: > Hi! This patch is broken out of the previous patch for all the builtins test > suite adjustments. Here we have some slight changes in error messages due to > how the internals have changed between the old and new builtins methods. > > For scalar-extract-exp-2.c we change: > error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration' > > to: > error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option > note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp' I don't like that at all. The user didn't write the _vsx thing, and it isn't documented either (neither is the _vec one, but that is a separate issue, specific to this builtin). > The new message provides more information. In both cases, it is less than > ideal that we don't refer to scalar_extract_exp, which is referenced in > the source line, but this is because scalar_extract_exp is #define'd to > __builtin_vec_scalar_extract_exp, so it's unavoidable. Certainly this is no > worse than before, and arguably better. It is a macro, enough said there The __builtin_ implementation should be documented (in the GCC manual, if not elsewhere). The warnings should talk about _vec, because the _vsx thing only exists as implementation detail, and we should never talk about those. We don't have errors about adddi3 either! > error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option > note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig' The rhs in the note does not *exist*, as far as the user is concerned. One builtin requiring another is all gobbledygook. > For scalar-test-neg-{2,3,5}.c, we actually change the test case. This is > because we deliberately removed some undocumented and pointless overloads, > where each overload mapped to a single builtin. These were: > __builtin_vec_scalar_test_neg_sp > __builtin_vec_scalar_test_neg_dp > __builtin_vec_scalar_test_neg_qp > which are redundant with the "real" overload: > __builtin_vec_scalar_test_neg > The latter maps to three builtins of the appropriate type. Yes. And the new ones are undocumented and useless just as well, they just have better names. Segher
On 11/17/21 3:32 PM, Segher Boessenkool wrote: > On Wed, Nov 17, 2021 at 02:58:54PM -0600, Bill Schmidt wrote: >> Hi! This patch is broken out of the previous patch for all the builtins test >> suite adjustments. Here we have some slight changes in error messages due to >> how the internals have changed between the old and new builtins methods. >> >> For scalar-extract-exp-2.c we change: >> error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration' >> >> to: >> error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option >> note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp' > I don't like that at all. The user didn't write the _vsx thing, and it > isn't documented either (neither is the _vec one, but that is a separate > issue, specific to this builtin). I feel like I haven't explained this well. This kind of thing has been in existence forever even in the old builtins code. The combination of the error showing the internal builtin name, and the note tying the overload name to the internal builtin name, has been there all along. The name of the internal builtin is pretty meaningless. The only thing that's interesting in this case is that we previously didn't get this *for this specific case* because the old code went to a generic fallback. But in many other cases you get exactly this same kind of error message for the old code. > >> The new message provides more information. In both cases, it is less than >> ideal that we don't refer to scalar_extract_exp, which is referenced in >> the source line, but this is because scalar_extract_exp is #define'd to >> __builtin_vec_scalar_extract_exp, so it's unavoidable. Certainly this is no >> worse than before, and arguably better. > It is a macro, enough said there > > The __builtin_ implementation should be documented (in the GCC manual, > if not elsewhere). The warnings should talk about _vec, because the > _vsx thing only exists as implementation detail, and we should never > talk about those. We don't have errors about adddi3 either! > >> error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option >> note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig' > The rhs in the note does not *exist*, as far as the user is concerned. > One builtin requiring another is all gobbledygook. As stated above, this isn't something new that I've added. That's what we already do. It's how the overload error messages have always been. I haven't been able to eradicate everything awful here... Thanks, Bill > >> For scalar-test-neg-{2,3,5}.c, we actually change the test case. This is >> because we deliberately removed some undocumented and pointless overloads, >> where each overload mapped to a single builtin. These were: >> __builtin_vec_scalar_test_neg_sp >> __builtin_vec_scalar_test_neg_dp >> __builtin_vec_scalar_test_neg_qp >> which are redundant with the "real" overload: >> __builtin_vec_scalar_test_neg >> The latter maps to three builtins of the appropriate type. > Yes. And the new ones are undocumented and useless just as well, they > just have better names. > > > Segher
Hi! On 11/17/21 5:06 PM, Bill Schmidt wrote: > On 11/17/21 3:32 PM, Segher Boessenkool wrote: >> On Wed, Nov 17, 2021 at 02:58:54PM -0600, Bill Schmidt wrote: >>> Hi! This patch is broken out of the previous patch for all the builtins test >>> suite adjustments. Here we have some slight changes in error messages due to >>> how the internals have changed between the old and new builtins methods. >>> >>> For scalar-extract-exp-2.c we change: >>> error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration' >>> >>> to: >>> error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option >>> note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp' >> I don't like that at all. The user didn't write the _vsx thing, and it >> isn't documented either (neither is the _vec one, but that is a separate >> issue, specific to this builtin). > I feel like I haven't explained this well. This kind of thing has been in > existence forever even in the old builtins code. The combination of the > error showing the internal builtin name, and the note tying the overload > name to the internal builtin name, has been there all along. The name of > the internal builtin is pretty meaningless. The only thing that's interesting > in this case is that we previously didn't get this *for this specific case* > because the old code went to a generic fallback. But in many other cases > you get exactly this same kind of error message for the old code. > >>> The new message provides more information. In both cases, it is less than >>> ideal that we don't refer to scalar_extract_exp, which is referenced in >>> the source line, but this is because scalar_extract_exp is #define'd to >>> __builtin_vec_scalar_extract_exp, so it's unavoidable. Certainly this is no >>> worse than before, and arguably better. >> It is a macro, enough said there >> >> The __builtin_ implementation should be documented (in the GCC manual, >> if not elsewhere). The warnings should talk about _vec, because the >> _vsx thing only exists as implementation detail, and we should never >> talk about those. We don't have errors about adddi3 either! >> >>> error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option >>> note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig' >> The rhs in the note does not *exist*, as far as the user is concerned. >> One builtin requiring another is all gobbledygook. > As stated above, this isn't something new that I've added. That's what > we already do. It's how the overload error messages have always been. That said, I don't like the "requires" language at all, either. How about replacing "builtin X requires builtin Y" with "overloaded builtin X is implemented by builtin Y" as a better explanation? That could be done easily with a standalone patch that doesn't affect the test suite, since none of the tests check for the note diagnostic. Thanks, Bill > > I haven't been able to eradicate everything awful here... > > Thanks, > Bill > >>> For scalar-test-neg-{2,3,5}.c, we actually change the test case. This is >>> because we deliberately removed some undocumented and pointless overloads, >>> where each overload mapped to a single builtin. These were: >>> __builtin_vec_scalar_test_neg_sp >>> __builtin_vec_scalar_test_neg_dp >>> __builtin_vec_scalar_test_neg_qp >>> which are redundant with the "real" overload: >>> __builtin_vec_scalar_test_neg >>> The latter maps to three builtins of the appropriate type. >> Yes. And the new ones are undocumented and useless just as well, they >> just have better names. >> >> >> Segher
Hi! On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote: > > I don't like that at all. The user didn't write the _vsx thing, and it > > isn't documented either (neither is the _vec one, but that is a separate > > issue, specific to this builtin). > > I feel like I haven't explained this well. This kind of thing has been in > existence forever even in the old builtins code. The combination of the > error showing the internal builtin name, and the note tying the overload > name to the internal builtin name, has been there all along. The name of > the internal builtin is pretty meaningless. The only thing that's interesting > in this case is that we previously didn't get this *for this specific case* > because the old code went to a generic fallback. But in many other cases > you get exactly this same kind of error message for the old code. Yes. And it still is a regression (in *this* case). Segher
On Thu, Nov 18, 2021 at 07:32:27AM -0600, Bill Schmidt wrote: > >>> error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option > >>> note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig' > >> The rhs in the note does not *exist*, as far as the user is concerned. > >> One builtin requiring another is all gobbledygook. > > As stated above, this isn't something new that I've added. That's what > > we already do. It's how the overload error messages have always been. > > That said, I don't like the "requires" language at all, either. How about > replacing "builtin X requires builtin Y" with "overloaded builtin X is > implemented by builtin Y" as a better explanation? That is better (although builtin Y *does not exist* as far as the user is concerned: it is not documented, and you cannot write it in source code afaics). Segher
On 11/18/21 3:16 PM, Segher Boessenkool wrote: > Hi! > > On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote: >>> I don't like that at all. The user didn't write the _vsx thing, and it >>> isn't documented either (neither is the _vec one, but that is a separate >>> issue, specific to this builtin). >> I feel like I haven't explained this well. This kind of thing has been in >> existence forever even in the old builtins code. The combination of the >> error showing the internal builtin name, and the note tying the overload >> name to the internal builtin name, has been there all along. The name of >> the internal builtin is pretty meaningless. The only thing that's interesting >> in this case is that we previously didn't get this *for this specific case* >> because the old code went to a generic fallback. But in many other cases >> you get exactly this same kind of error message for the old code. > Yes. And it still is a regression (in *this* case). Sorry, I don't understand. Why specifically is this a regression? Bill > > > Segher
On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote: > > On 11/18/21 3:16 PM, Segher Boessenkool wrote: > > Hi! > > > > On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote: > >>> I don't like that at all. The user didn't write the _vsx thing, and it > >>> isn't documented either (neither is the _vec one, but that is a separate > >>> issue, specific to this builtin). > >> I feel like I haven't explained this well. This kind of thing has been in > >> existence forever even in the old builtins code. The combination of the > >> error showing the internal builtin name, and the note tying the overload > >> name to the internal builtin name, has been there all along. The name of > >> the internal builtin is pretty meaningless. The only thing that's interesting > >> in this case is that we previously didn't get this *for this specific case* > >> because the old code went to a generic fallback. But in many other cases > >> you get exactly this same kind of error message for the old code. > > Yes. And it still is a regression (in *this* case). > > Sorry, I don't understand. Why specifically is this a regression? It is wrong now, in ways that it wasn't wrong before. That is the definition of regression! Segher
Hi! On 11/18/21 3:32 PM, Segher Boessenkool wrote: > On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote: >> On 11/18/21 3:16 PM, Segher Boessenkool wrote: >>> Hi! >>> >>> On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote: >>>>> I don't like that at all. The user didn't write the _vsx thing, and it >>>>> isn't documented either (neither is the _vec one, but that is a separate >>>>> issue, specific to this builtin). >>>> I feel like I haven't explained this well. This kind of thing has been in >>>> existence forever even in the old builtins code. The combination of the >>>> error showing the internal builtin name, and the note tying the overload >>>> name to the internal builtin name, has been there all along. The name of >>>> the internal builtin is pretty meaningless. The only thing that's interesting >>>> in this case is that we previously didn't get this *for this specific case* >>>> because the old code went to a generic fallback. But in many other cases >>>> you get exactly this same kind of error message for the old code. >>> Yes. And it still is a regression (in *this* case). >> Sorry, I don't understand. Why specifically is this a regression? > It is wrong now, in ways that it wasn't wrong before. That is the > definition of regression! I'm sorry, I disagree. With clarification of the note, I don't see how this can be considered a regression. It is providing information in a different way, but it is still clear that the user has misused the builtin in the context, and, unlike before, it now tells them *what* is wrong with the options that were used (not just "unavailable in this configuration"). The fact that an internal builtin name is *also* disclosed as part of this does not make it wrong. The way that overloads work, we can only tell whether a builtin is enabled with the current set of options by looking at the true builtin that the overload maps to. The enablement checking code doesn't have any knowledge that an overloaded function maps to it. So that error message is produced without knowledge of the overloading. The note diagnostic is added by the overload processing code that *is* aware that the mapping exists. The enablement checking code (rs6000_invalid_builtin in the old code, rs6000_invalid_new_builtin in the new code) is called from multiple places, and not always in an overload context, so we can't assume this is the case. Not all builtins are mapped to by overloads, but they still need enablement checking. Would it be possible to change things so that we pass in the overload name to be used in the error message when appropriate? Yes. But this would have a much larger impact on the test suite than the current method, because all error tests for overloads would now have to change. That is, there are many existing tests that are already "wrong" in the sense that they report the internal builtin name. I suggest that we add that to the list of future cleanups due to the size of the impact. I agree that it has never been ideal to mention the internal builtin name on these messages. It's just not unique to the changes I've made here; it's a pre-existing condition that needs work to cleanse it everywhere. Can we move forward this way? Thanks, Bill > > > Segher
Hi! I'd like to ping this patch. Segher had objected to the change in diagnostics, but I hope we've solved that now with the better informational message [1]. Thanks! Bill [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585250.html On 11/17/21 2:58 PM, Bill Schmidt wrote: > Hi! This patch is broken out of the previous patch for all the builtins test > suite adjustments. Here we have some slight changes in error messages due to > how the internals have changed between the old and new builtins methods. > > For scalar-extract-exp-2.c we change: > error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration' > > to: > error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option > note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp' > > The new message provides more information. In both cases, it is less than > ideal that we don't refer to scalar_extract_exp, which is referenced in > the source line, but this is because scalar_extract_exp is #define'd to > __builtin_vec_scalar_extract_exp, so it's unavoidable. Certainly this is no > worse than before, and arguably better. > > The cases for: > scalar-insert-exp-2.c > scalar-insert-exp-5.c > scalar-insert-exp-8.c > are all similar. > > For scalar-extract-sig-2.c we again change: > error: '__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration' > > to: > error: '__builtin_vsx_scalar_extract_sig' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option > note: builtin '__builtin_vec_scalar_extract_sig' requires builtin '__builtin_vsx_scalar_extract_sig' > > Here it is clearer because there is no #define to muddy things up, and > again the new message is arguably better than the old. > > For scalar-test-neg-{2,3,5}.c, we actually change the test case. This is > because we deliberately removed some undocumented and pointless overloads, > where each overload mapped to a single builtin. These were: > __builtin_vec_scalar_test_neg_sp > __builtin_vec_scalar_test_neg_dp > __builtin_vec_scalar_test_neg_qp > which are redundant with the "real" overload: > __builtin_vec_scalar_test_neg > The latter maps to three builtins of the appropriate type. > > The revised test case uses the "real" overload instead, and otherwise the > changes to the error messages are the same as for all the other cases. > > 2021-11-17 Bill Schmidt <wschmidt@linux.ibm.com> > > gcc/testsuite/ > * gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust error > message. > * gcc.target/powerpc/bfp/scalar-extract-sig-2.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-8.c: Likewise. > * gcc.target/powerpc/bfp/scalar-test-neg-2.c: Likewise. > * gcc.target/powerpc/bfp/scalar-test-neg-3.c: Likewise. > * gcc.target/powerpc/bfp/scalar-test-neg-5.c: Likewise. > --- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c | 2 +- > gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c | 2 +- > 8 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c > index 922180675fc..53b67c95cf9 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c > @@ -14,7 +14,7 @@ get_exponent (double *p) > { > double source = *p; > > - return scalar_extract_exp (source); /* { dg-error "'__builtin_vec_scalar_extract_exp' is not supported in this compiler configuration" } */ > + return scalar_extract_exp (source); /* { dg-error "'__builtin_vsx_scalar_extract_exp' requires the" } */ > } > > > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c > index e24d4bd23fe..39ee74c94dc 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c > @@ -12,5 +12,5 @@ get_significand (double *p) > { > double source = *p; > > - return __builtin_vec_scalar_extract_sig (source); /* { dg-error "'__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration" } */ > + return __builtin_vec_scalar_extract_sig (source); /* { dg-error "'__builtin_vsx_scalar_extract_sig' requires the" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c > index feb943104da..efd69725905 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c > @@ -16,5 +16,5 @@ insert_exponent (unsigned long long int *significand_p, > unsigned long long int significand = *significand_p; > unsigned long long int exponent = *exponent_p; > > - return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */ > + return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp' requires the" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c > index 0e5683d5d1a..f85966a6fdf 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c > @@ -16,5 +16,5 @@ insert_exponent (double *significand_p, > double significand = *significand_p; > unsigned long long int exponent = *exponent_p; > > - return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */ > + return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp_dp' requires the" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c > index bd68f770985..b1be8284b4e 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c > @@ -16,5 +16,5 @@ insert_exponent (unsigned __int128 *significand_p, /* { dg-error "'__int128' is > unsigned __int128 significand = *significand_p; /* { dg-error "'__int128' is not supported on this target" } */ > unsigned long long int exponent = *exponent_p; > > - return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */ > + return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp' requires the" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c > index 7d2b4deefc3..46d743a899b 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c > @@ -10,5 +10,5 @@ test_neg (float *p) > { > float source = *p; > > - return __builtin_vec_scalar_test_neg_sp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_sp' requires" } */ > + return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_sp' requires" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c > index b503dfa8b56..bfc892b116e 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c > @@ -10,5 +10,5 @@ test_neg (double *p) > { > double source = *p; > > - return __builtin_vec_scalar_test_neg_dp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_dp' requires" } */ > + return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_dp' requires" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c > index bab86040a7b..8c55c1cfb5c 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c > @@ -10,5 +10,5 @@ test_neg (__ieee128 *p) > { > __ieee128 source = *p; > > - return __builtin_vec_scalar_test_neg_qp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_qp' requires" } */ > + return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_qp' requires" } */ > }
On Thu, Nov 18, 2021 at 03:59:41PM -0600, Bill Schmidt wrote: > On 11/18/21 3:32 PM, Segher Boessenkool wrote: > > On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote: > >> On 11/18/21 3:16 PM, Segher Boessenkool wrote: > >>> On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote: > >>>>> I don't like that at all. The user didn't write the _vsx thing, and it > >>>>> isn't documented either (neither is the _vec one, but that is a separate > >>>>> issue, specific to this builtin). > >>>> I feel like I haven't explained this well. This kind of thing has been in > >>>> existence forever even in the old builtins code. The combination of the > >>>> error showing the internal builtin name, and the note tying the overload > >>>> name to the internal builtin name, has been there all along. The name of > >>>> the internal builtin is pretty meaningless. The only thing that's interesting > >>>> in this case is that we previously didn't get this *for this specific case* > >>>> because the old code went to a generic fallback. But in many other cases > >>>> you get exactly this same kind of error message for the old code. > >>> Yes. And it still is a regression (in *this* case). > >> Sorry, I don't understand. Why specifically is this a regression? > > It is wrong now, in ways that it wasn't wrong before. That is the > > definition of regression! > > I'm sorry, I disagree. With clarification of the note, I don't see how > this can be considered a regression. It is providing information in a > different way, but it is still clear that the user has misused the builtin > in the context, and, unlike before, it now tells them *what* is wrong with > the options that were used (not just "unavailable in this configuration"). > The fact that an internal builtin name is *also* disclosed as part of > this does not make it wrong. It is claiming something is wrong with something the user didn't write. The blow is softened a lot by the inform(), but it still is a regression. But you said you'll look into it, and it will be okay for now, it isn't like this is super important. Just an ugly wart :-) > The way that overloads work, we can only tell whether a builtin is > enabled with the current set of options by looking at the true builtin > that the overload maps to. The enablement checking code doesn't have > any knowledge that an overloaded function maps to it. So that error > message is produced without knowledge of the overloading. The note > diagnostic is added by the overload processing code that *is* aware > that the mapping exists. Yeah, but you can keep track of what the original name was, somewhere. Hopefully, anyway :-) > The enablement checking code (rs6000_invalid_builtin in the old code, > rs6000_invalid_new_builtin in the new code) is called from multiple > places, and not always in an overload context, so we can't assume > this is the case. Not all builtins are mapped to by overloads, but > they still need enablement checking. A direct call would not get the "this comes from <this> overload" field set, so that is easy to see in the error message code. > Would it be possible to change things so that we pass in the overload > name to be used in the error message when appropriate? Yes. But > this would have a much larger impact on the test suite than the > current method, because all error tests for overloads would now > have to change. That is, there are many existing tests that are > already "wrong" in the sense that they report the internal builtin > name. > > I suggest that we add that to the list of future cleanups due to > the size of the impact. I agree that it has never been ideal to > mention the internal builtin name on these messages. It's just > not unique to the changes I've made here; it's a pre-existing > condition that needs work to cleanse it everywhere. > > Can we move forward this way? We can yes :-) And thanks in advance! It would be nice if we could see this in GCC 12 still. It is fine for stage 4 still, if it is error message only (and a lot of testsuite :-) ) Segher
On Wed, Nov 17, 2021 at 02:58:54PM -0600, Bill Schmidt wrote: > Hi! This patch is broken out of the previous patch for all the builtins test > suite adjustments. Here we have some slight changes in error messages due to > how the internals have changed between the old and new builtins methods. > > For scalar-extract-exp-2.c we change: > error: '__builtin_vec_scalar_extract_exp is not supported in this compiler configuration' > > to: > error: '__builtin_vsx_scalar_extract_exp' requires the '-mcpu=power9' option and either the '-m64' or '-mpowerpc64' option > note: builtin '__builtin_vec_scalar_extract_exp' requires builtin '__builtin_vsx_scalar_extract_exp' So, okay for trunk. Thanks! One thing though: > gcc/testsuite/ > * gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust error > message. Don't wrap unnecessarily please. > * gcc.target/powerpc/bfp/scalar-extract-sig-2.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-8.c: Likewise. > * gcc.target/powerpc/bfp/scalar-test-neg-2.c: Likewise. > * gcc.target/powerpc/bfp/scalar-test-neg-3.c: Likewise. > * gcc.target/powerpc/bfp/scalar-test-neg-5.c: Likewise. Segher
diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c index 922180675fc..53b67c95cf9 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c @@ -14,7 +14,7 @@ get_exponent (double *p) { double source = *p; - return scalar_extract_exp (source); /* { dg-error "'__builtin_vec_scalar_extract_exp' is not supported in this compiler configuration" } */ + return scalar_extract_exp (source); /* { dg-error "'__builtin_vsx_scalar_extract_exp' requires the" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c index e24d4bd23fe..39ee74c94dc 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c @@ -12,5 +12,5 @@ get_significand (double *p) { double source = *p; - return __builtin_vec_scalar_extract_sig (source); /* { dg-error "'__builtin_vec_scalar_extract_sig' is not supported in this compiler configuration" } */ + return __builtin_vec_scalar_extract_sig (source); /* { dg-error "'__builtin_vsx_scalar_extract_sig' requires the" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c index feb943104da..efd69725905 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c @@ -16,5 +16,5 @@ insert_exponent (unsigned long long int *significand_p, unsigned long long int significand = *significand_p; unsigned long long int exponent = *exponent_p; - return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */ + return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp' requires the" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c index 0e5683d5d1a..f85966a6fdf 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c @@ -16,5 +16,5 @@ insert_exponent (double *significand_p, double significand = *significand_p; unsigned long long int exponent = *exponent_p; - return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */ + return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp_dp' requires the" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c index bd68f770985..b1be8284b4e 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c @@ -16,5 +16,5 @@ insert_exponent (unsigned __int128 *significand_p, /* { dg-error "'__int128' is unsigned __int128 significand = *significand_p; /* { dg-error "'__int128' is not supported on this target" } */ unsigned long long int exponent = *exponent_p; - return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vec_scalar_insert_exp' is not supported in this compiler configuration" } */ + return scalar_insert_exp (significand, exponent); /* { dg-error "'__builtin_vsx_scalar_insert_exp' requires the" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c index 7d2b4deefc3..46d743a899b 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c @@ -10,5 +10,5 @@ test_neg (float *p) { float source = *p; - return __builtin_vec_scalar_test_neg_sp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_sp' requires" } */ + return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_sp' requires" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c index b503dfa8b56..bfc892b116e 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-3.c @@ -10,5 +10,5 @@ test_neg (double *p) { double source = *p; - return __builtin_vec_scalar_test_neg_dp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_dp' requires" } */ + return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_dp' requires" } */ } diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c index bab86040a7b..8c55c1cfb5c 100644 --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c @@ -10,5 +10,5 @@ test_neg (__ieee128 *p) { __ieee128 source = *p; - return __builtin_vec_scalar_test_neg_qp (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_qp' requires" } */ + return __builtin_vec_scalar_test_neg (source); /* { dg-error "'__builtin_vsx_scalar_test_neg_qp' requires" } */ }