On Tue, Nov 04, 2014 at 12:07:56PM +0000, Joern Rennecke wrote: > On 31 October 2014 15:10, James Greenhalgh <james.greenha...@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
