This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch 3/7 arc] Deprecate *_BY_PIECES_P, move to hookized version


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]