public inbox for
 help / color / mirror / Atom feed
From: Jeff Law <>
To: Robin Dapp <>,
	Manolis Tsamis <>,,
	Philipp Tomsich <>,
	Jakub Jelinek <>,
Subject: Re: [PATCH v3 1/4] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets
Date: Fri, 10 Nov 2023 14:48:38 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

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


  reply	other threads:[~2023-11-10 21:48 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-30 10:13 [PATCH v3 0/4] ifcvt: Allow if conversion of arithmetic in basic blocks with multiple sets Manolis Tsamis
2023-08-30 10:13 ` [PATCH v3 1/4] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets Manolis Tsamis
2023-10-19 19:41   ` Richard Sandiford
2023-10-20  7:04     ` Robin Dapp
2023-10-20  9:16       ` Richard Sandiford
2023-11-10 21:48         ` Jeff Law [this message]
2023-11-10 21:31       ` Jeff Law
2023-11-10 21:25     ` Jeff Law
2024-04-23 10:58     ` Manolis Tsamis
2023-08-30 10:13 ` [PATCH v3 2/4] ifcvt: Allow more operations in multiple set if conversion Manolis Tsamis
2023-10-19 19:46   ` Richard Sandiford
2023-11-10 21:53     ` Jeff Law
2023-11-13 12:47     ` Manolis Tsamis
2024-04-23 11:00     ` Manolis Tsamis
2023-08-30 10:13 ` [PATCH v3 3/4] ifcvt: Handle multiple rewired regs and refactor noce_convert_multiple_sets Manolis Tsamis
2023-11-10 23:20   ` Jeff Law
2023-11-13 12:40     ` Manolis Tsamis
2023-11-21 18:10       ` Manolis Tsamis
2023-08-30 10:14 ` [PATCH v3 4/4] ifcvt: Remove obsolete code for subreg handling in noce_convert_multiple_sets Manolis Tsamis
2023-11-10 22:03   ` Jeff Law
2023-11-13 12:43     ` Manolis Tsamis
2023-11-21 18:08       ` Manolis Tsamis
2023-09-18  8:18 ` [PATCH v3 0/4] ifcvt: Allow if conversion of arithmetic in basic blocks with multiple sets Manolis Tsamis
2023-10-19  6:53   ` Manolis Tsamis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).