From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) by sourceware.org (Postfix) with ESMTPS id 41C773858D32; Tue, 13 Jun 2023 13:47:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 41C773858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ed1-x52e.google.com with SMTP id 4fb4d7f45d1cf-5186c90e3bbso2006566a12.3; Tue, 13 Jun 2023 06:47:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686664048; x=1689256048; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=zLM4ZdJvYBC/x7/RmpmKjWN0RETLuo+SXmRys9J/M/w=; b=VZEonnEwHAIqrh+a80TW71ie5zhTXmXbLlZbKs4+eI/BPVouELnLos8mkjjxsJfx4N N4+y5Zt9IRqm+su1st0txCdKPEDX8gunVw/+to1uLKXNoKPq0HFc2Xjr98WwKNIYOc6i cdpIh3Cn6ZoOFXPok/i73otpWJzO9prIRWQdu1kOsuNIe0b05HfGtDS+wSh0JJUmacNv FHIURqUavvyh2V6sj7R/QRXJCTKO6DYbt2djJEwR6j6l4VddyuEfcdAOZbNRKusFqveO kfBUMAdSUUFmh44OsqzSPa0zLNXFEeCcwYMNig1lFcvUSzwUxzWH2kcpYoPYmpsE1gLI EtcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686664048; x=1689256048; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zLM4ZdJvYBC/x7/RmpmKjWN0RETLuo+SXmRys9J/M/w=; b=hClEB993CsS5JHp8G7qAa2cVxvmkSxeAC8+IXq7DXZ4dY0tb1xPAY9Z3VsouT2kaCi NcRT+foPGX5bJfwEb9cUXecMqU0DCbwl0CbJnt/O1wflmXzp6HxxHOEFZaHdX4xLZTcG BRVv9lBwaUBV0HrSNXNtVnsU4kcNMCt5SMigsgD7dpBOxv0vUe/4Vze55y6JcpwH5qcJ 2nP2tl1MLQAWkNa6m8QaRncJm3HTOND2uD5GAKgNCqf+cTSEE/7FZJ7FJFq6xYmqzE02 qsUgWf9gpRb223j7lEcAhzZ71GhtR+juwCunz2d6vQQn7mpwOHYjcD/0dbVkgqHA0bXW 3xpA== X-Gm-Message-State: AC+VfDztOJAmOQ2ZfC8/JlJAk+Gomgvz4YmwB905+akYi4ZTvCu3jRF1 QOKuHhrySNjwnuK13t89Tr4STgTpXXqmmv+8kQDTIEu+ X-Google-Smtp-Source: ACHHUZ7HUMtN+kuUUv0u3x2DFD+NpqYc1gNOJ9ae2zUH8vyrIO04DjsTbw1N/Lu36rOs71cgVoPjP2JrU132M6NlFsk= X-Received: by 2002:a17:907:7207:b0:96f:d780:5734 with SMTP id dr7-20020a170907720700b0096fd7805734mr12941749ejc.65.1686664047511; Tue, 13 Jun 2023 06:47:27 -0700 (PDT) MIME-Version: 1.0 References: <20230608015547.3432691-1-guojiufu@linux.ibm.com> <20230608015547.3432691-2-guojiufu@linux.ibm.com> <7ncz20ca3n.fsf@ltcden2-lp1.aus.stglabs.ibm.com> In-Reply-To: <7ncz20ca3n.fsf@ltcden2-lp1.aus.stglabs.ibm.com> From: David Edelsohn Date: Tue, 13 Jun 2023 09:47:16 -0400 Message-ID: Subject: Re: [PATCH 1/4] rs6000: build constant via li;rotldi To: Jiufu Guo Cc: gcc-patches@gcc.gnu.org, segher@kernel.crashing.org, linkw@gcc.gnu.org, bergner@linux.ibm.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,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: 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 po= sitive > > 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): New= 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_rotl= di. > > > > 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' instructi= on > > + is able to load. If so, set *ROT to the number by which C is rota= ted, > > + 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 o= f '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', anot= her 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 w= ould > > 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 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. Thanks, David > > BR, > Jeff (Jiufu Guo) > > > > ... > > > > This is a style of software engineering, but it seems overkill to me wh= en the function is a single line that tail calls another function. Am I mi= ssing > > 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 & 0x= 8000))) > > @@ -10305,6 +10346,17 @@ rs6000_emit_set_long_const (rtx dest, HOST_WI= DE_INT c) > > emit_move_insn (dest, gen_rtx_XOR (DImode, temp, > > GEN_INT ((ud2 ^ 0xffff) << 16= ))); > > } > > + 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 - shif= t)); > > + > > + 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/test= suite/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