public inbox for
 help / color / mirror / Atom feed
From: Robin Dapp <>
To: 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, 20 Oct 2023 09:04:55 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

> 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

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


  reply	other threads:[~2023-10-20  7:04 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 [this message]
2023-10-20  9:16       ` Richard Sandiford
2023-11-10 21:48         ` Jeff Law
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).