Re: [PATCH v3 1/4] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets

[Robin Dapp <rdapp.gcc@gmail.com>]

> 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.

> This condition looks odd.  A SET_SRC is never null.  But more importantly,
> continuing means "assume the best", and I don't think we should assume
> the best for (say) an insn with two parallel sets.

IIRC I didn't consider two parallel sets at all :/

> But I'm not sure which cases this code is trying to catch.  Is it trying
> to catch cases where seq2 "spontaneously" uses registers that happen to
> overlap with cond?  If so, then when does that happen?  And if it does
> happen, wouldn't the sequence also have to set the registers first?

In order for sequence costing to be superior to just counting "conditional"
instructions we need to make sure that as few redundant instructions as
possible are present in the costed sequences. (redundant as in "will be
removed in a subsequent pass").
 
So, originally, the backend would always be passed a comparison
(set cc (cmp a b)).  On e.g. s390 this would result in at least two
redundant instructions per conditional move so costing was always wrong.
When passing the backend the comparison "result" e.g. (cmp cc 0)
there is no redundant instruction.

Now, passing the comparison is useful as well when we want to turn
a conditional move into a min/max in the backend.  This, however,
overwrites the initial condition result and any subsequent iterations
must not pass the result but the comparison itself to the backend.

In my first approach I hadn't considered several special cases which,
of course, complicated things further down the line.

All that said - maybe trying hard to get rid of later-redundant insns
is not a good approach after all?  A lot of complexity would go away
if we counted emitted conditional instructions just like the other
ifcvt part.  Maybe a middle ground would be to cost the initial
sequence as well as if + jmp and compare this against insn_cost of
only the created conditional insns?

In that case we might need to tighten/audit our preconditions in order
not to accidentally allow cases that result in strictly worse code.
But maybe that's an easier problem than what we are currently solving?

Regards
 Robin