| Message ID | 1414768204-28039-1-git-send-email-james.greenhalgh@arm.com |
|---|---|
| State | New |
| Headers | show |
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. Which, incidentally, this hookization completely ignores, entrenching the conflation of move expander and move cost estimates. 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. > > 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
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
On 4 November 2014 14:24, James Greenhalgh <james.greenhalgh@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.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?". 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?
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c index d04be01..c5b8b80 100644 --- a/gcc/config/arc/arc.c +++ b/gcc/config/arc/arc.c @@ -415,6 +415,11 @@ static void output_short_suffix (FILE *file); static bool arc_frame_pointer_required (void); +static bool arc_use_by_pieces_infrastructure_p (unsigned int, + unsigned int, + enum by_pieces_operation op, + bool); + /* Implements target hook vector_mode_supported_p. */ static bool @@ -530,6 +535,10 @@ static void arc_finalize_pic (void); #undef TARGET_DELEGITIMIZE_ADDRESS #define TARGET_DELEGITIMIZE_ADDRESS arc_delegitimize_address +#undef TARGET_USE_BY_PIECES_INFRASTRUCTURE_P +#define TARGET_USE_BY_PIECES_INFRASTRUCTURE_P \ + arc_use_by_pieces_infrastructure_p + /* Usually, we will be able to scale anchor offsets. When this fails, we want LEGITIMIZE_ADDRESS to kick in. */ #undef TARGET_MIN_ANCHOR_OFFSET @@ -9383,6 +9392,21 @@ arc_legitimize_reload_address (rtx *p, machine_mode mode, int opnum, return false; } +/* Implement TARGET_USE_BY_PIECES_INFRASTRUCTURE_P. */ + +static bool +arc_use_by_pieces_infrastructure_p (unsigned int size, + unsigned int align, + enum by_pieces_operation op, + bool speed_p) +{ + /* Let the movmem expander handle small block moves. */ + if (op == MOVE_BY_PIECES_P) + return false; + + return default_use_by_pieces_infrastructure_p (size, align, op, speed_p); +} + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-arc.h" diff --git a/gcc/config/arc/arc.h b/gcc/config/arc/arc.h index d40f5c3..2d27787 100644 --- a/gcc/config/arc/arc.h +++ b/gcc/config/arc/arc.h @@ -1553,12 +1553,6 @@ extern int arc_return_address_regs[4]; in one reasonably fast instruction. */ #define MOVE_MAX 4 -/* Let the movmem expander handle small block moves. */ -#define MOVE_BY_PIECES_P(LEN, ALIGN) 0 -#define CAN_MOVE_BY_PIECES(SIZE, ALIGN) \ - (move_by_pieces_ninsns (SIZE, ALIGN, MOVE_MAX_PIECES + 1) \ - < (unsigned int) MOVE_RATIO (!optimize_size)) - /* Undo the effects of the movmem pattern presence on STORE_BY_PIECES_P . */ #define MOVE_RATIO(SPEED) ((SPEED) ? 15 : 3)