Robin Dapp <rd...@linux.ibm.com> writes:
> Hi Richard,
>
> thanks for going through the patch set.
>
>> A hash_set might be simpler, given that the code only enters insns
>> for which the bool is false. “rtx_insn *” would be better than rtx.
>
> Right, changed that.
>
>> Do you mean the sets might be removed or that the checks might be
>> removed?
>
> It's actually the checks that are removed. I clarified and amended the
> comments.
>
>> The patch is quite hard to review on its own, since nothing actually
>> uses this variable. It's also not obvious how the
>> reg_overlap_mentioned_p code works if the old target is referenced
>> later.
>>
>> Could you refactor the series a bit so that each patch is
>> self-contained? It's OK if that means fewer patches.
> The attached v2 makes use of need_cmov now, I hope this makes it a bit
> clearer. Regarding the reg_overlap_mentioned_p: it is used to detect
> the swap idioms as well as when a cmov destination is used in the
> condition as well. If needed this could be two separate patches (most
> of the second patch would be comments, though).
Thanks for the update. No need for further splitting, this looks like a
nice self-contained patch as it is.
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 017944f4f79..a5e55456d88 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -98,6 +98,7 @@ static int dead_or_predicable (basic_block, basic_block,
> basic_block,
> edge, int);
> static void noce_emit_move_insn (rtx, rtx);
> static rtx_insn *block_has_only_trap (basic_block);
> +static void check_need_cmovs (basic_block, hash_set<rtx_insn *> *);
>
> /* Count the number of non-jump active insns in BB. */
>
> @@ -3203,6 +3204,10 @@ noce_convert_multiple_sets (struct noce_if_info
> *if_info)
> auto_vec<rtx_insn *> unmodified_insns;
> int count = 0;
>
> + hash_set<rtx_insn *> need_no_cmov;
> +
> + check_need_cmovs (then_bb, &need_no_cmov);
> +
> FOR_BB_INSNS (then_bb, insn)
> {
> /* Skip over non-insns. */
> @@ -3213,26 +3218,47 @@ noce_convert_multiple_sets (struct noce_if_info
> *if_info)
> gcc_checking_assert (set);
>
> rtx target = SET_DEST (set);
> - rtx temp = gen_reg_rtx (GET_MODE (target));
> + rtx temp;
> rtx new_val = SET_SRC (set);
> rtx old_val = target;
>
> - /* If we were supposed to read from an earlier write in this block,
> - we've changed the register allocation. Rewire the read. While
> - we are looking, also try to catch a swap idiom. */
> - for (int i = count - 1; i >= 0; --i)
> - if (reg_overlap_mentioned_p (new_val, targets[i]))
> - {
> - /* Catch a "swap" style idiom. */
> - if (find_reg_note (insn, REG_DEAD, new_val) != NULL_RTX)
> - /* The write to targets[i] is only live until the read
> - here. As the condition codes match, we can propagate
> - the set to here. */
> - new_val = SET_SRC (single_set (unmodified_insns[i]));
> - else
> - new_val = temporaries[i];
> - break;
> - }
Don't we still need this code (without the REG_DEAD handling) for the
case in which…
> + /* As we are transforming
> + if (x > y)
> + {
> + a = b;
> + c = d;
> + }
> + into
> + a = (x > y) ...
> + c = (x > y) ...
> +
> + we potentially check x > y before every set.
> + Even though the check might be removed by subsequent passes, this means
> + that we cannot transform
> + if (x > y)
> + {
> + x = y;
> + ...
> + }
> + into
> + x = (x > y) ...
> + ...
> + since this would invalidate x and the following to-be-removed checks.
> + Therefore we introduce a temporary every time we are about to
> + overwrite a variable used in the check. Costing of a sequence with
> + these is going to be inaccurate so only use temporaries when
> + needed. */
> + if (reg_overlap_mentioned_p (target, cond))
> + temp = gen_reg_rtx (GET_MODE (target));
…this code triggers? I don't see otherwise how later uses of x would
pick up “temp” instead of the original target. E.g. suppose we had:
if (x > y)
{
x = …;
z = x; // x does not die here
}
Without the loop, it looks like z would pick up the old value of x
(used in the comparison) instead of the new one.
> + else
> + temp = target;
> +
> + /* We have identified swap-style idioms in check_need_cmovs. A normal
> + set will need to be a cmov while the first instruction of a swap-style
> + idiom can be a regular move. This helps with costing. */
> + bool need_cmov = true;
> + if (need_no_cmov.contains (insn))
> + need_cmov = false;
Would be simpler as:
bool need_cmov = !need_no_cmov.contains (insn);
>
> /* If we had a non-canonical conditional jump (i.e. one where
> the fallthrough is to the "else" case) we need to reverse
> @@ -3275,9 +3301,22 @@ noce_convert_multiple_sets (struct noce_if_info
> *if_info)
> old_val = lowpart_subreg (dst_mode, old_val, src_mode);
> }
>
> - /* Actually emit the conditional move. */
> - rtx temp_dest = noce_emit_cmove (if_info, temp, cond_code,
> - x, y, new_val, old_val);
> + rtx temp_dest = NULL_RTX;
> +
> + if (need_cmov)
> + {
> + /* Actually emit the conditional move. */
> + temp_dest = noce_emit_cmove (if_info, temp, cond_code,
> + x, y, new_val, old_val);
> + }
> + else
> + {
> + if (if_info->then_else_reverse)
> + noce_emit_move_insn (temp, old_val);
> + else
> + noce_emit_move_insn (temp, new_val);
> + temp_dest = temp;
> + }
>
> /* If we failed to expand the conditional move, drop out and don't
> try to continue. */
I think this comment and the associated null check belong in the
“if (need_cmov)”
> @@ -3808,6 +3847,65 @@ check_cond_move_block (basic_block bb,
> return TRUE;
> }
>
> +/* Find local swap-style idioms in BB and mark the first insn (1)
> + that is only a temporary as not needing a conditional move as
> + it is going to be dead afterwards anyway.
> +
> + (1) int tmp = a;
> + a = b;
> + b = tmp;
> +
> + ifcvt
> + -->
> +
> + load tmp,a
> + cmov a,b
> + cmov b,tmp */
> +
> +static void
> +check_need_cmovs (basic_block bb, hash_set<rtx_insn *> *need_no_cmov)
> +{
> + rtx_insn *insn;
> + int count = 0;
> + auto_vec<rtx_insn *> insns;
> + auto_vec<rtx> dests;
> +
> + /* Iterate over all SETs, storing the destinations
> + in DEST. If we hit a SET that reads from a destination
> + that we have seen before and the corresponding register
> + is dead afterwards, it must be a swap. */
This is probably pedantic, but I guess it could also be a missed
forward-propagation opportunity. E.g. there's no reason in principle
why we couldn't see:
int tmp = a;
b = tmp; // tmp dies here
The code handles that case correctly, but it isn't a swap.
> + FOR_BB_INSNS (bb, insn)
> + {
> + rtx set, src, dest;
> +
> + if (!active_insn_p (insn))
> + continue;
> +
> + set = single_set (insn);
> + if (set == NULL_RTX)
> + continue;
> +
> + src = SET_SRC (set);
> + dest = SET_DEST (set);
> +
> + /* Check if the current SET's source is the same
> + as any previously seen destination.
> + This is quadratic but the number of insns in BB
> + is bounded by PARAM_MAX_RTL_IF_CONVERSION_INSNS. */
> + for (int i = count - 1; i >= 0; --i)
> + {
> + if (reg_overlap_mentioned_p (src, dests[i])
> + && find_reg_note (insn, REG_DEAD, src) != NULL_RTX)
> + need_no_cmov->add (insns[i]);
> + }
Minor formatting nit, sorry, but the braces aren't needed.
This is really a comment about the existing swap recognition code,
but I think the reg_overlap_mentioned_p check would be more obviously
correct if we guard the “for” loop with:
if (REG_P (src))
This guarantees that any previous instructions are equal to SRC or
subregs of it. I guess it might be more efficient too.
Thanks,
Richard
