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

Richard Sandiford richard.sandiford@arm.com
Tue Nov 30 16:36:32 GMT 2021


BTW, in response to your earlier concern about stage 3: you posted the
series well in time for end of stage 1, so I think it can still go in
during stage 3.

Robin Dapp <rdapp@linux.ibm.com> writes:
> 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.

Thanks, renaming the third function helps.

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

Still, perhaps we could at least add (in rtl.h):

struct rtx_comparison {
  rtx_code code;
  machine_mode op_mode;
  rtx op0, op1;
};

and make the existing emit_conditional_moves use it instead of four
separate parameters.  These rtx arguments would then be replacing those
rtx_comparison arguments, which would avoid the ambiguity in the overloads.

With C++ it should be possible to rewrite the calls using { … }, e.g.:

  if (!emit_conditional_move (into_target, { cmp_code, op1_mode, cmp1, cmp2 },
			      into_target, into_superword, word_mode, false))

so the new type wouldn't need to spread too far.

Does that sound OK?  If so, could you post the current version of full
patch series and say which bits still need review?

Thanks,
Richard


More information about the Gcc-patches mailing list