Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

[Robin Dapp <rdapp@linux.ibm.com>]

Hi Richard,

> It's hard to judge this in isolation because it's not clear when
> and how the new arguments are going to be used, but it seems OK
> in principle.  Do you still want:
> 
>   /* If earliest == jump, try to build the cmove insn directly.
>      This is helpful when combine has created some complex condition
>      (like for alpha's cmovlbs) that we can't hope to regenerate
>      through the normal interface.  */
> 
>   if (if_info->cond_earliest == if_info->jump)
>     {
> 
> to be used when cc_cmp and rev_cc_cmp are nonnull?

My initial hunch was to just leave it in place as I did not manage to
trigger it.  As it is going to be called and costed both ways (with
cc_cmp, rev_cc_cmp and without) it is probably better to move it into
the else branch.

The single usage of this is in patch 5/7.  We are passing the already
existing condition from the jump and its reverse to see if the backend
can come up with something better than when creating a new comparison.

>> +static rtx emit_conditional_move (rtx, rtx, rtx, rtx, machine_mode);
>> +rtx emit_conditional_move (rtx, rtx, rtx, rtx, rtx, machine_mode);
> 
> This is redundant with the header file declaration.
> 

Removed it.

> I think it'd be better to call one of these functions something else,
> rather than make the interpretation of the third parameter depend on
> the total number of parameters.  In the second overload, the comparison
> rtx effectively replaces four parameters of the existing
> emit_conditional_move, so perhaps that's the one that should remain
> emit_conditional_move.  Maybe the first one should be called
> emit_conditional_move_with_rev or something.

Not entirely fond of calling the first one _with_rev because essentially
both try normal and reversed variants but I agree that the naming is not
ideal.  I don't have any great ideas how to properly untangle it so I
would go with your suggestions in order to move forward.  As there is
only one caller of the second function, we could also let the caller
handle the reversing.  Then, the third function would need to be
non-static, though.

The third, static emit_conditional_move I already renamed locally to
emit_conditional_move_1.

> Part of me wonders if this would be simpler if we created a structure
> to describe a comparison and passed that around instead of individual
> fields, but I guess it could become a rat hole.

I also thought about this as it would allow us to use either
representation as required by the usage site.  Even tried it in a branch
locally but indeed it became ugly quickly so I postponed it for now.

Regards
 Robin