On 05/01/16 13:42, Bernd Schmidt wrote:
On 12/21/2015 10:23 AM, Kyrill Tkachov wrote:
Here is a version integrating the new checks into bbs_ok_for_cmove_arith.
We might as well do it there since it already iterates over all the
instructions in the basic blocks.
The patch looks somewhat complicated and asymmetrical to me. I tried to debug
the issue, and found that bbs_ok_for_cmove_arith wasn't called at all. After
changing that, like this:
@@ -2082,7 +2082,7 @@ noce_try_cmove_arith (struct noce_if_inf
}
}
- if (then_bb && else_bb && !a_simple && !b_simple
+ if (then_bb && else_bb
&& (!bbs_ok_for_cmove_arith (then_bb, else_bb)
|| !bbs_ok_for_cmove_arith (else_bb, then_bb)))
return FALSE;
the testcase passes. Could you investigate whether this is sufficient?
This works around the issue but we don't want to do perform the check for pairs
of
simple basic blocks because then we'll end up rejecting code that does things
like:
x = cond ? x + 1 : x - 1
i.e. source of the set in both blocks reads and writes the same register.
We can deal with this safely later on in the function since we rename the
destinations
of the two sets, so we don't want to reject this case here.
So your proposal rejects this case on that basis, which is overly restrictive
and doesn't
deal with the underlying issue of the condition code being clobbered.
I had tried this approach initially but it caused code quality regressions on
SPEC.
* ifcvt.c (cond_exec_get_condition): Rename to...
(get_condition_from_jump): ... This.
Please don't.
Ok, I'll leave it as cond_exec_get_condition
+/* { dg-options "-Og -fif-conversion -flive-range-shrinkage -fpeel-loops
-frerun-cse-after-loop" } */
That's a very specific set of options there, I wonder how Zdenek even found
that. Maybe we also want a copy in torture/ to test it more broadly?
Judging from some other reports from Zdenek that I've seen I think he tries a
matrix of options, getting some
of the exotic combinations.
So should we have a copy in gcc.dg/ with the explicit options and a clean copy
in torture?
Thanks,
Kyrill
Bernd
