This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [m68k 05/13] improve 64bit shift handling
- From: Jeffrey Law <law at redhat dot com>
- To: zippel at linux-m68k dot org
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Tue, 06 Feb 2007 11:27:35 -0700
- Subject: Re: [m68k 05/13] improve 64bit shift handling
- References: <20070130112615.782382000@linux-m68k.org> <20070130114606.866698000@linux-m68k.org>
- Reply-to: law at redhat dot com
On Tue, 2007-01-30 at 12:26 +0100, zippel@linux-m68k.org wrote:
> Hi,
>
> Originially this patch did fix one of the reload problems, but in the
> meantime a slightly different patch went in, so this patch mainly cleans
> up the 64bit shift handling.
> It fixes many predicates to avoid unnecessary reloads, also the
> expander definitions only accept register arguments and leaves
> generating memory arguments (plus possible scratch register) to combine.
> It also splits many constant shift operations into separate rtl
> instructions where possible.
>
>
> 2007-01-30 Roman Zippel <zippel@linux-m68k.org>
>
> * config/m68k/m68k.c (split_di): New.
> * config/m68k/m68k-protos.h: Declare split_di.
> * config/m68k/m68k.md (ashldi3*,ashrdi3*,lshrdi3*):
> Improve predicate handling and split constant shifts.
This looks pretty good. I did spot checking on the new splits to
verify that they look reasonable relative to the raw code we used
to generate. I'm going to assume the subreg word numbers are
correct (they looked reasonable, but I didn't look at each and
every one closely).
A few minor comments:
1. It might have been better to split this patch up. Yes
it's fairly mechanical in the sense that you applied the
same transformation to each of three 64bit shit patterns.
Submitting each one separately would have been a little
easier to review.
2. You need a function comment before split_di. Also in
split_di s/refuse/refuses/
3. You modified extendsidi2, but it's not mentioned in the
ChangeLog. DId you mean to include it? (Arguably it
could have been in its own patch).
4. Do you need a CC_STATUS_INIT in your new *ashrdi3_const1?
Similarly for the other shift by one patterns.
5. Are the CC status bits correct when we split something
like a 32bit shift into moves?
Assuming #4 and #5 aren't problems that require changing the
functionality of your patch, then consider the patch pre-approved
after you address issues #2 and #3.
jeff