| Message ID | 568BF08E.1010900@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Thanks for the help, Kyrill > > Bernd
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. Kyrill > Thanks for the help, > Kyrill > >> >> Bernd >
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: gcc/ifcvt.c =================================================================== --- gcc/ifcvt.c (revision 232056) +++ gcc/ifcvt.c (working copy) @@ -1866,11 +1866,13 @@ insn_valid_noce_process_p (rtx_insn *ins } -/* Return true iff the registers that the insns in BB_A set do not - get used in BB_B. */ +/* Return true iff the registers that the insns in BB_A set do not get + used in BB_B. WILL_RENAME is true if we expect that BB_A consists + of a single single_set insn with a destination that the caller will + rename to a new pseudo. */ static bool -bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b) +bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b, bool will_rename) { rtx_insn *a_insn; bitmap bba_sets = BITMAP_ALLOC (®_obstack); @@ -1887,13 +1889,15 @@ bbs_ok_for_cmove_arith (basic_block bb_a if (!sset_a) { + gcc_assert (!will_rename); BITMAP_FREE (bba_sets); return false; } /* Record all registers that BB_A sets. */ FOR_EACH_INSN_DEF (def, a_insn) - bitmap_set_bit (bba_sets, DF_REF_REGNO (def)); + if (!will_rename || DF_REF_REG (def) != SET_DEST (sset_a)) + bitmap_set_bit (bba_sets, DF_REF_REGNO (def)); } rtx_insn *b_insn; @@ -2082,9 +2086,9 @@ noce_try_cmove_arith (struct noce_if_inf } } - if (then_bb && else_bb && !a_simple && !b_simple - && (!bbs_ok_for_cmove_arith (then_bb, else_bb) - || !bbs_ok_for_cmove_arith (else_bb, then_bb))) + if (then_bb && else_bb + && (!bbs_ok_for_cmove_arith (then_bb, else_bb, a_simple) + || !bbs_ok_for_cmove_arith (else_bb, then_bb, b_simple))) return FALSE; start_sequence ();