This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch 3/7 arc] Deprecate *_BY_PIECES_P, move to hookized version
- From: James Greenhalgh <james dot greenhalgh at arm dot com>
- To: Joern Rennecke <joern dot rennecke at embecosm dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 4 Nov 2014 14:24:07 +0000
- Subject: Re: [Patch 3/7 arc] Deprecate *_BY_PIECES_P, move to hookized version
- Authentication-results: sourceware.org; auth=none
- References: <1414768100-27840-1-git-send-email-james dot greenhalgh at arm dot com> <1414768204-28039-1-git-send-email-james dot greenhalgh at arm dot com> <CAMqJFCqHcmGx1AN=HiiHNxGFjq7wVWmin5M06zXPRY8hjtCBCQ at mail dot gmail dot com>
On Tue, Nov 04, 2014 at 12:07:56PM +0000, Joern Rennecke wrote:
> On 31 October 2014 15:10, James Greenhalgh <james.greenhalgh@arm.com> wrote:
>
> > While I am there, arc defines a macro CAN_MOVE_BY_PIECES, which is
> > unused, so clean that up too.
>
> That's not a clean-up. This pertains to PR 39350.
Well, it is a clean-up in the sense that this macro is completely unused
in the compiler and has no effect, but please revert this hunk if that
is your preference.
> Which, incidentally, this hookization completely ignores, entrenching
> the conflation of move expander and move cost estimates.
No, I have to disagree. The use_by_pieces_infrastructure_p hook doesn't
conflate anything - it gives a response to the question "Should the
by_pieces infrastructure be used?". A target specific movmem pattern
- though it might itself choose to move things by pieces, is
categorically not using the move_by_pieces infrastructure.
If we want to keep a clean separation of concerns here, we would
want a similar target hook asking the single question "will your
movmem/setmem expander succeed?".
> Thus, can_move_by_pieces gives the wrong result for purposes of rtl
> optimizations
> when a target-specific movmem etc expander emits target-specific code.
> The patch at https://gcc.gnu.org/ml/gcc-patches/2009-03/txt00018.txt
> shows a number of call sites that are affected.
can_move_by_pieces (likewise can_store_by_pieces) gives the right
result, the RTL expanders are using it wrong.
I disagree with the approach taken in your patch as it overloads the
purpose of can_move_by_pieces. However, I would support a patch pulling
this out in to two hooks, so the call in
value-prof.c:gimple_stringops_transform would change from:
if (!can_move_by_pieces (val, MIN (dest_align, src_align)))
return false;
to something like:
if (!can_move_by_pieces (val, MIN (dest_align, src_align))
&& !targetm.can_expand_mem_op_p (val, MIN (dest_align, src_align),
MOVE_BY_PIECES))
return false;
But let's not confuse the use of what should be a simple hook!
> > arc only implements MOVE_BY_PIECES_P, wiring it to false. Mirror that
> > behaviour, and use the default hook for other by_pieces operations.
> >
> > I tried building a compiler but no amount of fiddling with target
> > strings got me to a sensible result, so this patch is completely
> > untested.
>
> You could just pick one of the configs in contrib/config-list.mk
Digging in to this, my scripts like to integrate a GDB build - which
doesn't work for arc-unknown-elf. I've been following the builds on
Jan Benedict-Glaw's since I put the patches in on Saturday morning,
and it doesn't look like I broke anything for arc. If I have any more
arc patches I'll keep this in mind.
Thanks,
James