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

[Jeff Law <jeffreyalaw@gmail.com>]

On 10/20/23 03:16, Richard Sandiford wrote:
> Thanks for the context.
> 
> Robin Dapp <rdapp.gcc@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