2011/10/26 Jiangning Liu <jiangning....@arm.com>: > > >> -----Original Message----- >> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- >> ow...@gcc.gnu.org] On Behalf Of Michael Matz >> Sent: Tuesday, October 11, 2011 10:45 PM >> To: Kai Tietz >> Cc: Richard Guenther; Kai Tietz; gcc-patches@gcc.gnu.org; Richard >> Henderson >> Subject: Re: [patch tree-optimization]: Improve handling of >> conditional-branches on targets with high branch costs >> >> Hi, >> >> On Tue, 11 Oct 2011, Kai Tietz wrote: >> >> > > Better make it a separate function the first tests your new >> > > conditions, and then calls simple_operand_p. >> > >> > Well, either I make it a new function and call it instead of >> > simple_operand_p, >> >> That's what I meant, yes. >> >> > >> @@ -5149,13 +5176,6 @@ fold_truthop (location_t loc, enum tree_ >> > >> build2 (BIT_IOR_EXPR, TREE_TYPE (ll_arg), >> > >> ll_arg, rl_arg), >> > >> build_int_cst (TREE_TYPE (ll_arg), 0)); >> > >> - >> > >> - if (LOGICAL_OP_NON_SHORT_CIRCUIT) >> > >> - { >> > >> - if (code != orig_code || lhs != orig_lhs || rhs != >> orig_rhs) >> > >> - return build2_loc (loc, code, truth_type, lhs, rhs); >> > >> - return NULL_TREE; >> > >> - } >> > > >> > > Why do you remove this hunk? Shouldn't you instead move the hunk >> you >> > > added to fold_truth_andor() here. I realize this needs some TLC to >> > > fold_truth_andor_1, because right now it early-outs for non- >> comparisons, >> > > but it seems the better place. I.e. somehow move the below code >> into the >> > > above branch, with the associated diddling on fold_truth_andor_1 >> that it >> > > gets called. >> > >> > This hunk is removed, as it is vain to do here. >> >> There is a fallthrough now, that wasn't there before. I don't know if >> it's harmless, I just wanted to mention it. >> > > Yes, this part introduced different behavior for this small case, > > int f(char *i, int j) > { > if (*i && j!=2) > return *i; > else > return j; > } > > Before the fix, we have > > D.4710 = *i; > D.4711 = D.4710 != 0; > D.4712 = j != 2; > D.4713 = D.4711 & D.4712; > if (D.4713 != 0) goto <D.4714>; else goto <D.4715>; > <D.4714>: > D.4710 = *i; > D.4716 = (int) D.4710; > return D.4716; > <D.4715>: > D.4716 = j; > return D.4716; > > After the fix, we have > > D.4711 = *i; > if (D.4711 != 0) goto <D.4712>; else goto <D.4710>; > <D.4712>: > if (j != 2) goto <D.4713>; else goto <D.4710>; > <D.4713>: > D.4711 = *i; > D.4714 = (int) D.4711; > return D.4714; > <D.4710>: > D.4714 = j; > return D.4714; > > Does this meet the original expectation? > > I find the code below in function fold_truth_andor makes difference, > > /* Transform (A AND-IF B) into (A AND B), or (A OR-IF B) into (A OR B). > For sequence point consistancy, we need to check for trapping, > and side-effects. */ > else if (code == icode && simple_operand_p_2 (arg0) > && simple_operand_p_2 (arg1)) > return fold_build2_loc (loc, ncode, type, arg0, arg1); > > for "*i != 0" simple_operand_p(*i) returns false. Originally this is not > checked by the code. I don't see the patch originally wanted to cover this. > Can this be explained reasonably? > > I'm not arguing this patch did worse thing, but only want to understand the > rationale behind this and justify this patch doesn't hurt anything else. > Actually on the contrary, I measured and this change accidently made some > benchmarks significantly improved due to some other reasons. > > Thanks, > -Jiangning > >> > Btw richi asked for it, and I agree that new TRUTH-AND/OR packing is >> > better done at a single place in fold_truth_andor only. >> >> As fold_truthop is called twice by fold_truth_andor, the latter might >> indeed be the better place. >> >> >> Ciao, >> Michael.
Well, as far as I understand C specification and sequence points, it makes indeed a difference to do branch-cost optimization on instructions might cause a signal traps. As signal could be handled in specifc handlers. You need to consider here that short-circuit optimization assumes associative law on operands. So right-hand side might be evaluaded before the left-hand-side. And this indeed breaks IMHO the sequences as mentioned in C specification about conditions. So a memory de-referencing (same as a floating-point trap) can never be treated as simple, as far as I understood this. So this patch - beside improving logic for branch-cost merging - fixes this latent issue. Regards, Kai