Message ID | 20181114130752.5057-3-rdapp@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | If conversion with multiple sets. | expand |
On 11/14/18 6:07 AM, Robin Dapp wrote: > This patch checks whether the current target supports conditional moves > with immediate then/else operands and allows noce_convert_multiple_sets > to deal with constants subsequently. > Also, minor refactoring is performed. > > -- > > gcc/ChangeLog: > > 2018-11-14 Robin Dapp <rdapp@linux.ibm.com> > > * ifcvt.c (have_const_cmov): New function. > (noce_convert_multiple_sets): Allow constants if supported. > (bb_ok_for_noce_convert_multiple_sets): Likewise. > (check_cond_move_block): Refactor. > --- > gcc/ifcvt.c | 46 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 36 insertions(+), 10 deletions(-) > > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c > index ddf077fa051..660bb46eb1c 100644 > --- a/gcc/ifcvt.c > +++ b/gcc/ifcvt.c > @@ -3077,6 +3077,27 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond, > return false; > } > > +/* Check if we have a movcc pattern that accepts constants as then/else > + operand (op 2/3). */ > +static bool > +have_const_cmov (machine_mode mode) > +{ > + enum insn_code icode; > + if ((icode = direct_optab_handler (movcc_optab, mode)) > + != CODE_FOR_nothing) > + { > + if (insn_data[(int) icode].operand[2].predicate > + && (insn_data[(int) icode].operand[2].predicate > + (const1_rtx, insn_data[(int) icode].operand[2].mode))) > + if (insn_data[(int) icode].operand[3].predicate > + && (insn_data[(int) icode].operand[3].predicate > + (const1_rtx, insn_data[(int) icode].operand[3].mode))) > + return true; > + } > + > + return false; > +} This may ultimately be too simplistic. There are targets where some constants are OK, but others may not be. By checking the predicate like this I think you can cause over-aggressive if-conversion if the target allows a range of integers in the expander's operand predicate, but allows a narrower range in the actual define_insn (presumably the expander loads them into a pseudo or somesuch in that case). We know that over-aggressive if-conversion into conditional moves hurts some targets. Ideally you'd create the actual insn with the constants you want to use and see if that's recognized as well as compute its cost. Is that just too painful at this point for some reason? > @@ -3689,7 +3717,7 @@ check_cond_move_block (basic_block bb, > { > rtx set, dest, src; > > - if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn)) > + if (!active_insn_p (insn)) > continue; > set = single_set (insn); > if (!set) > @@ -3705,10 +3733,8 @@ check_cond_move_block (basic_block bb, > if (!CONSTANT_P (src) && !register_operand (src, VOIDmode)) > return FALSE; > > - if (side_effects_p (src) || side_effects_p (dest)) > - return FALSE; > - > - if (may_trap_p (src) || may_trap_p (dest)) > + /* Check for side effects and trapping. */ > + if (!noce_operand_ok (src) || !noce_operand_ok (dest)) > return FALSE; > > /* Don't try to handle this if the source register was These two hunks are probably OK as general cleanups. Note that noce_operand_ok is not strictly the same as checking side_effects_p and may_trap_p in the case of a MEM. But you've already filtered out MEMs before you get here. Jeff >
> This may ultimately be too simplistic. There are targets where some > constants are OK, but others may not be. By checking the predicate > like this I think you can cause over-aggressive if-conversion if the > target allows a range of integers in the expander's operand predicate, > but allows a narrower range in the actual define_insn (presumably the > expander loads them into a pseudo or somesuch in that case). > > We know that over-aggressive if-conversion into conditional moves hurts > some targets. > > Ideally you'd create the actual insn with the constants you want to use > and see if that's recognized as well as compute its cost. Is that just > too painful at this point for some reason? Conditional moves in noce_convert_multiple_sets are processed via noce_emit_cmove which itself performs quite some preprocessing and calls optab's emit_conditional_move in the end, so the insn will already be emitted before being able to decide whether to decline it due to costs. In addition, every noce_emit_cmove will emit the condition check again as well as possible temporaries. Comparing the costs of the whole sequence will therefore still prove difficult as all the additionally generated insns will not get removed until reload and make a fair comparison nigh impossible. I was reluctant to factor out all the preprocessing stuff, separate it from the condition check and actual emitting part but that's properly the "right way" to do it, including emitting the condition only once in the beginning. However, for now, I could imagine changing only the conversion_profitable_p hook in our backend to only count the cmovs and ignore everything else in the sequence. This would be somewhat hacky though and still wouldn't allow constants :) Regards Robin
On 11/15/18 4:34 AM, Robin Dapp wrote: >> This may ultimately be too simplistic. There are targets where some >> constants are OK, but others may not be. By checking the predicate >> like this I think you can cause over-aggressive if-conversion if the >> target allows a range of integers in the expander's operand predicate, >> but allows a narrower range in the actual define_insn (presumably the >> expander loads them into a pseudo or somesuch in that case). >> >> We know that over-aggressive if-conversion into conditional moves hurts >> some targets. >> >> Ideally you'd create the actual insn with the constants you want to use >> and see if that's recognized as well as compute its cost. Is that just >> too painful at this point for some reason? > > Conditional moves in noce_convert_multiple_sets are processed via > noce_emit_cmove which itself performs quite some preprocessing and calls > optab's emit_conditional_move in the end, so the insn will already be > emitted before being able to decide whether to decline it due to costs. > In addition, every noce_emit_cmove will emit the condition check again > as well as possible temporaries. I wonder if we could arrange to emit them onto a new sequence. We could then cost the entire sequence. If we end up wanting to do the transformation we then splice that sequence into the main insn chain at the appropriate place. There should be a start/push/pop/end for sequences that you could use for helpers. > > Comparing the costs of the whole sequence will therefore still prove > difficult as all the additionally generated insns will not get removed > until reload and make a fair comparison nigh impossible. You'd probably have to make an educated guess about what insns from the original sequence would die and could therefore be removed. > > I was reluctant to factor out all the preprocessing stuff, separate it > from the condition check and actual emitting part but that's properly > the "right way" to do it, including emitting the condition only once in > the beginning. If the push_to_sequence idea works you may not have to do a ton of refactoring. Essentially it changes where things like emit_insn and friends splice in newly created insns. It may allow you to leave most of the current code in-place and you just have to splice in the sequence at the end (there's a routine to help you with that too -- it's been a long time, so the names aren't in memory anymore). > > However, for now, I could imagine changing only the > conversion_profitable_p hook in our backend to only count the cmovs and > ignore everything else in the sequence. This would be somewhat hacky > though and still wouldn't allow constants :) That does sound hacky.. jeff
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index ddf077fa051..660bb46eb1c 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -3077,6 +3077,27 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond, return false; } +/* Check if we have a movcc pattern that accepts constants as then/else + operand (op 2/3). */ +static bool +have_const_cmov (machine_mode mode) +{ + enum insn_code icode; + if ((icode = direct_optab_handler (movcc_optab, mode)) + != CODE_FOR_nothing) + { + if (insn_data[(int) icode].operand[2].predicate + && (insn_data[(int) icode].operand[2].predicate + (const1_rtx, insn_data[(int) icode].operand[2].mode))) + if (insn_data[(int) icode].operand[3].predicate + && (insn_data[(int) icode].operand[3].predicate + (const1_rtx, insn_data[(int) icode].operand[3].mode))) + return true; + } + + return false; +} + /* We have something like: if (x > y) @@ -3194,7 +3215,12 @@ noce_convert_multiple_sets (struct noce_if_info *if_info) we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI). Wrap the two cmove operands into subregs if appropriate to prevent that. */ - if (GET_MODE (new_val) != GET_MODE (temp)) + + /* Check if we can emit a cmove with constant operands. */ + bool allow_constants = have_const_cmov (GET_MODE (target)); + + if (!(allow_constants && CONST_INT_P (new_val)) + && GET_MODE (new_val) != GET_MODE (temp)) { machine_mode src_mode = GET_MODE (new_val); machine_mode dst_mode = GET_MODE (temp); @@ -3205,7 +3231,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info) } new_val = lowpart_subreg (dst_mode, new_val, src_mode); } - if (GET_MODE (old_val) != GET_MODE (temp)) + if (!(allow_constants && CONST_INT_P (old_val)) + && GET_MODE (old_val) != GET_MODE (temp)) { machine_mode src_mode = GET_MODE (old_val); machine_mode dst_mode = GET_MODE (temp); @@ -3339,9 +3366,10 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb) if (!REG_P (dest)) return false; - if (!(REG_P (src) - || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src)) - && subreg_lowpart_p (src)))) + if (!((REG_P (src) + || (have_const_cmov (GET_MODE (dest)) && CONST_INT_P (src))) + || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src)) + && subreg_lowpart_p (src)))) return false; /* Destination must be appropriate for a conditional write. */ @@ -3689,7 +3717,7 @@ check_cond_move_block (basic_block bb, { rtx set, dest, src; - if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn)) + if (!active_insn_p (insn)) continue; set = single_set (insn); if (!set) @@ -3705,10 +3733,8 @@ check_cond_move_block (basic_block bb, if (!CONSTANT_P (src) && !register_operand (src, VOIDmode)) return FALSE; - if (side_effects_p (src) || side_effects_p (dest)) - return FALSE; - - if (may_trap_p (src) || may_trap_p (dest)) + /* Check for side effects and trapping. */ + if (!noce_operand_ok (src) || !noce_operand_ok (dest)) return FALSE; /* Don't try to handle this if the source register was