From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id DD0FC3858CDA; Wed, 14 Jun 2023 01:16:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DD0FC3858CDA Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linux.ibm.com Received: from pps.filterd (m0353722.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 35E0peuY018099; Wed, 14 Jun 2023 01:16:46 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=from : to : cc : subject : references : date : in-reply-to : message-id : mime-version : content-type : content-transfer-encoding; s=pp1; bh=ADtdGsW9gQQKQ2WTzdYIJMxDHlOrj65VMFlVx915SfA=; b=PlmwvqReQ1/X67LKOto0Hk1Jq5XQvRC8q2D4tbS9GBWgbL0USQ3V92k23s/KB/ACYScP 1nhk2aGw0KKeePtNo1h9VDIXjLrbTWJCP+s2pAtQ/xnsDkxOjwkt94uVjflJi9FCFxUK SUK8CBHbuLpNODuev7Ce2lXm3OdGdn8696VuYWuTkBatyPU97YWxlPFw9COkEe65/uVL /cXc99wdyHzYIDJQJMFBuZn0iTzakXewgHyWC3MUk2u2W9StIhvCOOojgU5SuCB1v3Js lQx62l7QtRu2QmLvkcTNsw1oQb+k7KC8rQnxFe6vFCPmyThYV3VNWolQaPVtCOXQTVO2 3g== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3r73cnrg5g-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 14 Jun 2023 01:16:45 +0000 Received: from m0353722.ppops.net (m0353722.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 35E15ZDv027200; Wed, 14 Jun 2023 01:16:45 GMT Received: from ppma03wdc.us.ibm.com (ba.79.3fa9.ip4.static.sl-reverse.com [169.63.121.186]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3r73cnrg59-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 14 Jun 2023 01:16:45 +0000 Received: from pps.filterd (ppma03wdc.us.ibm.com [127.0.0.1]) by ppma03wdc.us.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 35DLFWuL002813; Wed, 14 Jun 2023 01:16:44 GMT Received: from smtprelay06.dal12v.mail.ibm.com ([9.208.130.100]) by ppma03wdc.us.ibm.com (PPS) with ESMTPS id 3r4gt5k3uk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 14 Jun 2023 01:16:44 +0000 Received: from smtpav03.dal12v.mail.ibm.com (smtpav03.dal12v.mail.ibm.com [10.241.53.102]) by smtprelay06.dal12v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 35E1GiVP63767018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 14 Jun 2023 01:16:44 GMT Received: from smtpav03.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 0A3845803F; Wed, 14 Jun 2023 01:16:44 +0000 (GMT) Received: from smtpav03.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D69C85805A; Wed, 14 Jun 2023 01:16:43 +0000 (GMT) Received: from ltcden2-lp1.aus.stglabs.ibm.com (unknown [9.3.90.43]) by smtpav03.dal12v.mail.ibm.com (Postfix) with ESMTPS; Wed, 14 Jun 2023 01:16:43 +0000 (GMT) From: Jiufu Guo To: David Edelsohn Cc: gcc-patches@gcc.gnu.org, segher@kernel.crashing.org, linkw@gcc.gnu.org, bergner@linux.ibm.com Subject: Re: [PATCH 1/4] rs6000: build constant via li;rotldi References: <20230608015547.3432691-1-guojiufu@linux.ibm.com> <20230608015547.3432691-2-guojiufu@linux.ibm.com> <7ncz20ca3n.fsf@ltcden2-lp1.aus.stglabs.ibm.com> Date: Wed, 14 Jun 2023 09:16:41 +0800 In-Reply-To: (David Edelsohn's message of "Tue, 13 Jun 2023 09:47:16 -0400") Message-ID: <7nlegmc07a.fsf@ltcden2-lp1.aus.stglabs.ibm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-TM-AS-GCONF: 00 X-Proofpoint-GUID: 12kwWIbM4MIcuza3kKwTieYHFblZfY71 X-Proofpoint-ORIG-GUID: mGIWMe6S4R0-VjVcdDrmZmtuhG4rQ2yy X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26 definitions=2023-06-13_24,2023-06-12_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 adultscore=0 priorityscore=1501 spamscore=0 impostorscore=0 suspectscore=0 bulkscore=0 phishscore=0 mlxlogscore=999 lowpriorityscore=0 clxscore=1015 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000 definitions=main-2306140006 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,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 List-Id: Hi, David Edelsohn writes: > On Mon, Jun 12, 2023 at 11:30=E2=80=AFPM Jiufu Guo wrote: >> >> >> Hi David, >> >> David Edelsohn writes: >> > On Wed, Jun 7, 2023 at 9:55=E2=80=AFPM Jiufu Guo wrote: >> > >> > Hi, >> > >> > This patch checks if a constant is possible to be rotated to/from a p= ositive >> > or negative value from "li". If so, we could use "li;rotldi" to build= it. >> > >> > Bootstrap and regtest pass on ppc64{,le}. >> > Is this ok for trunk? >> > >> > BR, >> > Jeff (Jiufu) >> > >> > gcc/ChangeLog: >> > >> > * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): Ne= w function. >> > (can_be_rotated_to_negative_li): New function. >> > (can_be_built_by_li_and_rotldi): New function. >> > (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rot= ldi. >> > >> > gcc/testsuite/ChangeLog: >> > >> > * gcc.target/powerpc/const-build.c: New test. >> > --- >> > gcc/config/rs6000/rs6000.cc | 64 +++++++++++++++++= -- >> > .../gcc.target/powerpc/const-build.c | 54 ++++++++++++++++ >> > 2 files changed, 112 insertions(+), 6 deletions(-) >> > create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c >> > >> > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >> > index 42f49e4a56b..1dd0072350a 100644 >> > --- a/gcc/config/rs6000/rs6000.cc >> > +++ b/gcc/config/rs6000/rs6000.cc >> > @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source) >> > return true; >> > } >> > >> > +/* Check if C can be rotated to a positive value which 'li' instruct= ion >> > + is able to load. If so, set *ROT to the number by which C is rot= ated, >> > + and return true. Return false otherwise. */ >> > + >> > +static bool >> > +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot) >> > +{ >> > + /* 49 leading zeros and 15 low bits on the positive value >> > + generated by 'li' instruction. */ >> > + return can_be_rotated_to_lowbits (c, 15, rot); >> > +} >> > + >> > +/* Like can_be_rotated_to_positive_li, but check the negative value = of 'li'. */ >> > + >> > +static bool >> > +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot) >> > +{ >> > + return can_be_rotated_to_lowbits (~c, 15, rot); >> > +} >> > + >> > +/* Check if value C can be built by 2 instructions: one is 'li', ano= ther is >> > + rotldi. >> > + >> > + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and = *MASK >> > + is set to -1, and return true. Return false otherwise. */ >> > + >> > >> > I look at this feature and it's good, but I don't fully understand the= benefit of this level of abstraction. Ideally all of the above functions = would >> > be inlined. They aren't reused. >> > >> > +static bool >> > +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift, >> > + HOST_WIDE_INT *mask) >> > +{ >> > + int n; >> > + if (can_be_rotated_to_positive_li (c, &n) >> > + || can_be_rotated_to_negative_li (c, &n)) >> > >> > Why not >> > >> > /* Check if C or ~C can be rotated to a positive or negative value >> > which 'li' instruction is able to load. */ >> > if (can_be_rotated_to_lowbits (c, 15, &n) >> > || can_be_rotated_to_lowbits (~c, 15, &n)) >> >> >> Thanks a lot for your review!! >> >> Your suggestions could also achieve my goal of using a new function: >> Using "can_be_rotated_to_positive_li" is just trying to get a >> straightforward name. Like yours, the code's comments would also >> make it easy to understand. > > I recognize that you are trying to be consistent with the other > functions that you add in later patches, but it feels like overkill in Yes :) > abstraction to me. Or maybe combine postive_li and negative_li into a > single function so that the abstraction serves a purpose other than a > tail call and creating an alias for a specific invocation of > can_be_rotated_to_lowbits. Get it. Thanks for your valuable suggestion! BR, Jeff (Jiufu Guo) > > Thanks, David > >> >> BR, >> Jeff (Jiufu Guo) >> > >> > ... >> > >> > This is a style of software engineering, but it seems overkill to me w= hen the function is a single line that tail calls another function. Am I m= issing >> > something? >> > >> > The rest of this patch looks good. >> > >> > Thanks, David >> > >> > + { >> > + *mask =3D HOST_WIDE_INT_M1; >> > + *shift =3D HOST_BITS_PER_WIDE_INT - n; >> > + return true; >> > + } >> > + >> > + return false; >> > +} >> > + >> > /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode. >> > Output insns to set DEST equal to the constant C as a series of >> > lis, ori and shl instructions. */ >> > @@ -10266,15 +10308,14 @@ static void >> > rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c) >> > { >> > rtx temp; >> > + int shift; >> > + HOST_WIDE_INT mask; >> > HOST_WIDE_INT ud1, ud2, ud3, ud4; >> > >> > ud1 =3D c & 0xffff; >> > - c =3D c >> 16; >> > - ud2 =3D c & 0xffff; >> > - c =3D c >> 16; >> > - ud3 =3D c & 0xffff; >> > - c =3D c >> 16; >> > - ud4 =3D c & 0xffff; >> > + ud2 =3D (c >> 16) & 0xffff; >> > + ud3 =3D (c >> 32) & 0xffff; >> > + ud4 =3D (c >> 48) & 0xffff; >> > >> > if ((ud4 =3D=3D 0xffff && ud3 =3D=3D 0xffff && ud2 =3D=3D 0xffff &= & (ud1 & 0x8000)) >> > || (ud4 =3D=3D 0 && ud3 =3D=3D 0 && ud2 =3D=3D 0 && ! (ud1 & 0= x8000))) >> > @@ -10305,6 +10346,17 @@ rs6000_emit_set_long_const (rtx dest, HOST_W= IDE_INT c) >> > emit_move_insn (dest, gen_rtx_XOR (DImode, temp, >> > GEN_INT ((ud2 ^ 0xffff) << 1= 6))); >> > } >> > + else if (can_be_built_by_li_and_rotldi (c, &shift, &mask)) >> > + { >> > + temp =3D !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode); >> > + unsigned HOST_WIDE_INT imm =3D (c | ~mask); >> > + imm =3D (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shi= ft)); >> > + >> > + emit_move_insn (temp, GEN_INT (imm)); >> > + if (shift !=3D 0) >> > + temp =3D gen_rtx_ROTATE (DImode, temp, GEN_INT (shift)); >> > + emit_move_insn (dest, temp); >> > + } >> > else if (ud3 =3D=3D 0 && ud4 =3D=3D 0) >> > { >> > temp =3D !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode); >> > diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c b/gcc/tes= tsuite/gcc.target/powerpc/const-build.c >> > new file mode 100644 >> > index 00000000000..70f095f6bf2 >> > --- /dev/null >> > +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c >> > @@ -0,0 +1,54 @@ >> > +/* { dg-do run } */ >> > +/* { dg-options "-O2 -save-temps" } */ >> > +/* { dg-require-effective-target has_arch_ppc64 } */ >> > + >> > +#define NOIPA __attribute__ ((noipa)) >> > + >> > +struct fun >> > +{ >> > + long long (*f) (void); >> > + long long val; >> > +}; >> > + >> > +long long NOIPA >> > +li_rotldi_1 (void) >> > +{ >> > + return 0x7531000000000LL; >> > +} >> > + >> > +long long NOIPA >> > +li_rotldi_2 (void) >> > +{ >> > + return 0x2100000000000064LL; >> > +} >> > + >> > +long long NOIPA >> > +li_rotldi_3 (void) >> > +{ >> > + return 0xffff8531ffffffffLL; >> > +} >> > + >> > +long long NOIPA >> > +li_rotldi_4 (void) >> > +{ >> > + return 0x21ffffffffffff94LL; >> > +} >> > + >> > +struct fun arr[] =3D { >> > + {li_rotldi_1, 0x7531000000000LL}, >> > + {li_rotldi_2, 0x2100000000000064LL}, >> > + {li_rotldi_3, 0xffff8531ffffffffLL}, >> > + {li_rotldi_4, 0x21ffffffffffff94LL}, >> > +}; >> > + >> > +/* { dg-final { scan-assembler-times {\mrotldi\M} 4 } } */ >> > + >> > +int >> > +main () >> > +{ >> > + for (int i =3D 0; i < sizeof (arr) / sizeof (arr[0]); i++) >> > + if ((*arr[i].f) () !=3D arr[i].val) >> > + __builtin_abort (); >> > + >> > + return 0; >> > +} >> > -- >> > 2.39.1