On 4 November 2014 14:24, James Greenhalgh <james.greenha...@arm.com> wrote: > 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?".
That would not be helpful. What the rtl optimizers actually want to know is "will this block copy / memset be cheap?" . A movmem expander might succeed (or not) for various reasons. The one that's interesting for the above question is if the call has been inlined with a fast set of instructions. >> 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 could agree with that view if there was a good strategy agreed what the rtl expanders should do instead. > 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 this goes back to the problem that it's not about if we can expand the mem op at all, but if we have a fast expansion. We can always expand via libcall (the middle end does this as a fall-back). Also, the target might do some target-specific slow expansion, e.g. call a function with another name and maybe a modified ABI, but still relatively slow to work. So, either the new hook would answer the wrong question, or it would be misnamed, in which case it's likely that the semantics will sooner or later follow the name. it will gravitate to answer the wrong question again. > But let's not confuse the use of what should be a simple hook! What would that be? TARGET_RTX_COST is unsuitable because the RTL for the call hasn't been made yet, and it it was, it would tend to be multiple instructions, maybe even a loop. Should we have an analogous TARGET_TREE_COST hook, so that you can ask the target what it thinks the cost of a tree will be once it's expanded?
