[PATCH] simplify-rtx: Extend (truncate (*extract ...)) fold [PR87763]
Jeff Law
law@redhat.com
Fri Jan 31 17:49:00 GMT 2020
On Thu, 2020-01-30 at 17:54 +0000, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
> > On Wed, 2020-01-29 at 19:18 +0000, Richard Sandiford wrote:
> > > Andreas Schwab <schwab@suse.de> writes:
> > > > On Jan 27 2020, Richard Sandiford wrote:
> > > >
> > > > > * simplify-rtx.c (simplify_truncation): Extend sign/zero_extract
> > > > > simplification to handle subregs as well as bare regs.
> > > >
> > > > That breaks gcc.target/m68k/pr39726.c
> > >
> > > Gah. Jeff pointed out off-list that it also broke
> > > gcc.target/sh/pr64345-1.c on sh3-linux-gnu. It didn't look like either
> > > of them would be fixable in an acceptably non-invasive and unhacky way,
> > > so I've reverted the patch "for now".
> > I would have considered letting those two targets regress those tests
> > to move forward on 87763. aarch64 is (IMHO) more important than the sh
> > and m68k combined ;-) It also seems to me that your patch generates
> > better RTL and that we could claim that a port that regresses on code
> > quality needs its port maintainer to step in and fix the port.
> >
> > WRT the m68k issue I'd think it could be fixed by twiddling
> > cbranchsi4_btst_reg_insn_1 to accept a mode iterator on the
> > zero_extract and making some minor adjustments in its output code.
> > Something like the attached. I haven't tested it in any real way and
> > haven't really thought about whether or not it does the right thing for
> > a MEM operand.
>
> It just feels like this is breaking the contract with extv and extzv,
> where all *_extracts are supposed to be in the mode that those patterns
> accept. My i386 patch was doing that too TBH, I was just being
> optimistic given that QImode was already handled by that pattern. :-)
>
> So IMO my patch has too many knock-on effects for it to be suitable for
> stage 4. While we have make_extraction, we probably have to leave this
> kind of expression untouched.
>
> For AArch64 I was planning on adding a new pattern to match the
> (subreg:SI (zero_extract:DI ...)) -- as one of the comments in the
> PR suggested -- but with the subreg matched via subreg_lowpart_operator,
> to make it endian-safe. This is similar in spirit to the new i686
> popcount patterns.
Fair enough on the simplify-rtx change :-) One might argue that we
should be loosening the requirements, but memory operands are
particularly troublesome. I think HP and I had a light discussion on
that a year or so ago.
WRT adding patterns to aarch64. Yea, I went that route last year, but
never was able to go from "hey, this seems to work pretty well" to
"it's OK for the trunk".
Jeff
More information about the Gcc-patches
mailing list