On 10/20/23 03:16, Richard Sandiford wrote:
Thanks for the context.
Robin Dapp <rdapp....@gmail.com> writes:
Sorry for the slow review. TBH I was hoping someone else would pick
it up, since (a) I'm not very familiar with this code, and (b) I don't
really agree with the way that the current code works. I'm not sure the
current dependency checking is safe, so I'm nervous about adding even
more cases to it. And it feels like the different ifcvt techniques are
not now well distinguished, so that they're beginning to overlap and
compete with one another. None of that is your fault, of course. :)
I might be to blame, at least partially :) The idea back then was to
do it here because (1) it can handle cases the other part cannot and
(2) its costing is based on sequence cost rather than just counting
instructions.
Ah, OK. (2) seems like a good reason.
Agreed. It's been a problem area (costing ifcvt), but it's still the
right thing to do. No doubt if we change something from counting insns
to sequence costing it'll cause another set of problems, but that
shouldn't stop us from doing the right thing here.
Yeah, makes sense. Using your example, there seem to be two things
that we're checking:
(1) Does the sequence change cc? This is checked via:
if (cc_cmp)
{
/* Check if SEQ can clobber registers mentioned in
cc_cmp and/or rev_cc_cmp. If yes, we need to use
only seq1 from that point on. */
rtx cc_cmp_pair[2] = { cc_cmp, rev_cc_cmp };
for (walk = seq; walk; walk = NEXT_INSN (walk))
{
note_stores (walk, check_for_cc_cmp_clobbers, cc_cmp_pair);
if (cc_cmp_pair[0] == NULL_RTX)
{
cc_cmp = NULL_RTX;
rev_cc_cmp = NULL_RTX;
break;
}
}
}
and is the case that Manolis's patch is dealing with.
(2) Does the sequence use a and b? If so, we need to use temporary
destinations for any earlier writes to a and b.
Is that right?
(1) looks OK, although Manolis's modified_in_p would work too.
Agreed.
(2) is the code I quoted yesterday and is the part I'm not sure
about. First of all:
seq1 = try_emit_cmove_seq (if_info, temp, cond,
new_val, old_val, need_cmov,
&cost1, &temp_dest1);
must have a consistent view of what a and b are. So old_val and new_val
cannot at this point reference "newer" values of a and b (as set by previous
instructions in the sequence). AIUI:
Sigh. ifcvt seems to pervasively adjust arguments, then you have to
figure out which one is the right one for any given context. I was
driving me nuts a couple weeks ago when I was looking at the condzero
work. It's part of why I set everything down at the time. I ran into
it in the VRULL code, Ventana's hacks on top of the VRULL code and in
the ESWIN code, got frustrated and decided to look at something else for
a bit (which has led to its own little rathole).
The same cond, new_val and old_val are used in:
seq2 = try_emit_cmove_seq (if_info, temp, cond,
new_val, old_val, need_cmov,
&cost2, &temp_dest2, cc_cmp, rev_cc_cmp);
So won't any use of a and b in seq2 to be from cond, rather than old_val
and new_val? If so, shouldn't we set read_comparison for any use of a
and b, rather than skipping IF_THEN_ELSE?
Seems like it to me, yes.
Using seq_cost seems like the best way of costing things. And I agree
that it's worth trying to avoid costing (and generating) redundant
instructions.
I think there's general agreement on seq_cost. I wonder if we should
look to split that out on its own, then figure out what to do with the
bigger issues in this space.
Jeff
