From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id CF5253858D37; Wed, 21 Jun 2023 11:51:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CF5253858D37 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1687348292; bh=o3dZeGu03RgwXfUp40y+s0GwX82qDOwJg5RtBNyJ2bg=; h=From:To:Subject:Date:In-Reply-To:References:From; b=lea97f7tNfRuNeasURSK7jdWmxMulpOaIrFiJvEPcWYP3l1LJjV50xJ3HLRpkMo1w yQjKf9F7ougVT11K5np5mCPJRKH6a18UNxKFnWfP4b89rthoUKRCKSfpuquWiMH6Y/ QvVEM2gU3jIjAuq+954I5sZdGfvtPbVnsfccoaO0= From: "klepikov.alex+bugs at gmail dot com" To: gcc-bugs@gcc.gnu.org Subject: [Bug target/54089] [SH] Refactor shift patterns Date: Wed, 21 Jun 2023 11:51:30 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: target X-Bugzilla-Version: 4.8.0 X-Bugzilla-Keywords: X-Bugzilla-Severity: enhancement X-Bugzilla-Who: klepikov.alex+bugs at gmail dot com X-Bugzilla-Status: ASSIGNED X-Bugzilla-Resolution: X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: olegendo at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D54089 --- Comment #86 from Alexander Klepikov --- (In reply to Oleg Endo from comment #85) >For now, can you try to run the style check script on your changes? Thank you, that's what I've been missing! > > bool > > expand_ashiftrt (rtx *operands) > > { > > int value =3D INTVAL (operands[2]) & 31; > ^^^^^^ >=20 > Missing check for 'const_int' operand. See other uses of 'CONST_INT_P'. I was completely sure that condition '(match_operand:SI 2 "const_int_operan= d")' in define_insn_and_split guarantees that 'CONST_INT_P (operands[2]) =3D=3D = true'. > I think the 'define_insn "ashrsi3_libcall_collapsed"' and > 'define_insn_and_split "ashrsi3_libcall_expand"' can be merged into a sin= gle > pattern 'define_insn_and_split "ashrsi3_n_call" and the function > 'expand_ashrsi3_libcall' (the tail of the original 'expand_ashiftrt') can= be > just put into the splitter code in the new "ashrsi3_n_call". >=20 > The splitter condition should include "&& can_create_pseudo_p ()" to make > sure it's ran before register allocation. >=20 > I think this will reduce the change set a bit and make the intention of t= he > patch a bit clearer. >=20 Yes, it looks like it works in general. But first tests show that all const= ant dynamic shifts expand to library call for all targets, even for those with dynshift instructions. That's because 'ashrsi3_n_call' and 'ashrsi3' should check whether right shift could be expanded to individual shifts. They both should execute the code that I separated into 'expand_ashiftrt_individual' function to expand to individual shifts properly. > > +/* { dg-final { scan-assembler "_f_loop1_rshift:.*mov\.l\\t(\.L\[0-9\]= +),(r\[0-9\]+).*sts.l\\tpr,@-r15.*(\.L\[0-9\]+):.*jsr\\t@\\2.*bf\.s\\t\\3.*= \\1:\\n\\t\.long\\t___ashiftrt_r4_6.*_f_loop2_rshift:" { target { ! has_dyn= _shift } } } } */ >=20 > Can you try to somehow write this in a simpler way? Maybe omit some of t= he > register number matches, as they don't matter etc. I took a note, I'll deal with it later.=