This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68841: Make sure one basic block doesn't clobber CC reg usage of the other


Hi Bernd,

On 05/01/16 18:19, Kyrill Tkachov wrote:

On 05/01/16 17:06, Kyrill Tkachov wrote:

On 05/01/16 16:34, Bernd Schmidt wrote:
On 01/05/2016 03:22 PM, Kyrill Tkachov wrote:

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 we need to teach bbs_ok_for_cmove_arith that this is going to happen. How about the approach below? Still seems to fix the issue, and it looks like the CC set is present in the df info so everything should work as intended. Right?


Yeah, this looks like it works.
However, now we reject if-conversion whereas with my patch we still tried switching the order in which
the blocks were emitted, which allowed for a bit more aggressive if-conversion.
I don't know if this approach is overly restrictive yet.
I'll try its effects on codegen quality on SPEC as soon as I get some cycles.
But this approach does look appealing to me.


Hmm, from a first look at SPEC, it seems to still overly restrict ifconversion in the
x = cond ? x + 1 : x - 1 case.
I'll look deeper tomorrow as to what's going on there.


Ok, found the problem.
bbs_ok_for_cmove_arith also checks that we don't perform any stores in the basic block, which kills opportunities
to convert operations of the form [addr] = c ? a : b. Since with your patch we now call this even for
simple basic blocks we miss these opportunities.

bb_valid_for_noce_process_p called earlier should have already ensured that for complex
blocks we allow only the last set to be a store, so we should be able to relax the restriction in
bbs_ok_for_cmove_arith. That change in combination with your patch fixes the testcase and has no code quality
fallout that I can see (it even slightly increases if-conversion opportunities). I'll test more thoroughly
and post a patch in due time.

Thanks,
Kyrill

Kyrill

Thanks for the help,
Kyrill


Bernd




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]