Ping ^2? Thanks! -Zhenqiang
On 28 May 2014 15:02, Zhenqiang Chen <zhenqiang.c...@linaro.org> wrote: > Ping? > > Thanks! > -Zhenqiang > > On 22 May 2014 17:52, Zhenqiang Chen <zhenqiang.c...@linaro.org> wrote: >> On 21 May 2014 20:43, Steven Bosscher <stevenb....@gmail.com> wrote: >>> On Wed, May 21, 2014 at 11:58 AM, Zhenqiang Chen wrote: >>>> Hi, >>>> >>>> The patch fixes the gcc.target/i386/pr49095.c FAIL in PR61225. The >>>> test case tends to check a peephole2 optimization, which optimizes the >>>> following sequence >>>> >>>> 2: bx:SI=ax:SI >>>> 25: ax:SI=[bx:SI] >>>> 7: {ax:SI=ax:SI-0x1;clobber flags:CC;} >>>> 8: [bx:SI]=ax:SI >>>> 9: flags:CCZ=cmp(ax:SI,0) >>>> to >>>> 2: bx:SI=ax:SI >>>> 41: {flags:CCZ=cmp([bx:SI]-0x1,0);[bx:SI]=[bx:SI]-0x1;} >>>> >>>> The enhanced shrink-wrapping, which calls copyprop_hardreg_forward >>>> changes the INSN 25 to >>>> >>>> 25: ax:SI=[ax:SI] >>>> >>>> Then peephole2 can not optimize it since two memory_operands look like >>>> different. >>>> >>>> To fix it, the patch adds another peephole2 rule to read one more >>>> insn. From the register copy, it knows the address is the same. >>> >>> That is one complex peephole2 to deal with a transformation like this. >>> It seems to be like it's a too specific solution for a bigger problem. >>> >>> Could you please try one of the following solutions instead: >>> >>> 1. Track register values for peephole2 and try different alternatives >>> based on known register equivalences? E.g. in your example, perhaps >>> there is already a REG_EQUAL/REG_EQUIV note available on insn 25 after >>> copyprop_hardreg_forward, to annotate that [ax:SI] is equivalent to >>> [bx:SI] at that point (or if that information is not available, it is >>> not very difficult to make it available). Then you could try applying >>> peephole2 on the original pattern but also on patterns modified with >>> the known equivalences (i.e. try peephole2 on multiple equivalent >>> patterns for the same insn). This may expose other peephole2 >>> opportunities, not just the specific one your patch addresses. >> >> Patch is updated according to the comment. There is no REG_EQUAL. So I >> add it when replace_oldest_value_reg. >> >> ChangeLog: >> 2014-05-22 Zhenqiang Chen <zhenqiang.c...@linaro.org> >> >> Part of PR rtl-optimization/61225 >> * config/i386/i386-protos.h (ix86_peephole2_rtx_equal_p): New proto. >> * config/i386/i386.c (ix86_peephole2_rtx_equal_p): New function. >> * regcprop.c (replace_oldest_value_reg): Add REG_EQUAL note when >> propagating to SET. >> >> diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h >> index 39462bd..0c4a2b9 100644 >> --- a/gcc/config/i386/i386-protos.h >> +++ b/gcc/config/i386/i386-protos.h >> @@ -42,6 +42,7 @@ extern enum calling_abi ix86_function_type_abi >> (const_tree); >> >> extern void ix86_reset_previous_fndecl (void); >> >> +extern bool ix86_peephole2_rtx_equal_p (rtx, rtx, rtx, rtx); >> #ifdef RTX_CODE >> extern int standard_80387_constant_p (rtx); >> extern const char *standard_80387_constant_opcode (rtx); >> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >> index 6ffb788..583ebe8 100644 >> --- a/gcc/config/i386/i386.c >> +++ b/gcc/config/i386/i386.c >> @@ -46856,6 +46856,29 @@ ix86_atomic_assign_expand_fenv (tree *hold, >> tree *clear, tree *update) >> atomic_feraiseexcept_call); >> } >> >> +/* OP0 is the SET_DEST of INSN and OP1 is the SET_SRC of INSN. >> + Check whether OP1 and OP6 is equal. */ >> + >> +bool >> +ix86_peephole2_rtx_equal_p (rtx insn, rtx op0, rtx op1, rtx op6) >> +{ >> + rtx note; >> + >> + if (!reg_overlap_mentioned_p (op0, op1) && rtx_equal_p (op1, op6)) >> + return true; >> + >> + gcc_assert (single_set (insn) >> + && op0 == SET_DEST (single_set (insn)) >> + && op1 == SET_SRC (single_set (insn))); >> + >> + note = find_reg_note (insn, REG_EQUAL, NULL_RTX); >> + if (note >> + && !reg_overlap_mentioned_p (op0, XEXP (note, 0)) >> + && rtx_equal_p (XEXP (note, 0), op6)) >> + return true; >> + >> + return false; >> +} >> /* Initialize the GCC target structure. */ >> #undef TARGET_RETURN_IN_MEMORY >> #define TARGET_RETURN_IN_MEMORY ix86_return_in_memory >> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md >> index 44e80ec..b57fc86 100644 >> --- a/gcc/config/i386/i386.md >> +++ b/gcc/config/i386/i386.md >> @@ -16996,11 +16996,12 @@ >> [(match_dup 0) >> (match_operand:SWI 2 "<nonmemory_operand>")])) >> (clobber (reg:CC FLAGS_REG))]) >> - (set (match_dup 1) (match_dup 0)) >> + (set (match_operand:SWI 6 "memory_operand") (match_dup 0)) >> (set (reg FLAGS_REG) (compare (match_dup 0) (const_int 0)))] >> "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ()) >> && peep2_reg_dead_p (4, operands[0]) >> - && !reg_overlap_mentioned_p (operands[0], operands[1]) >> + && ix86_peephole2_rtx_equal_p (peep2_next_insn (0), operands[0], >> + operands[1], operands[6]) >> && !reg_overlap_mentioned_p (operands[0], operands[2]) >> && (<MODE>mode != QImode >> || immediate_operand (operands[2], QImode) >> diff --git a/gcc/regcprop.c b/gcc/regcprop.c >> index 7a5a4f6..4e09724 100644 >> --- a/gcc/regcprop.c >> +++ b/gcc/regcprop.c >> @@ -510,6 +510,22 @@ replace_oldest_value_reg (rtx *loc, enum >> reg_class cl, rtx insn, >> fprintf (dump_file, "insn %u: replaced reg %u with %u\n", >> INSN_UID (insn), REGNO (*loc), REGNO (new_rtx)); >> >> + if (single_set (insn) && GET_CODE (PATTERN (insn)) == SET >> + && GET_MODE_CLASS (GET_MODE (SET_DEST (insn))) != MODE_CC >> + && GET_CODE (SET_SRC (single_set (insn))) != COMPARE) >> + { >> + rtx set = single_set (insn); >> + rtx dest = SET_DEST (set); >> + >> + if (REG_P (dest) && REG_P (new_rtx) >> + && REGNO (dest) == REGNO (new_rtx)) >> + /* REGNO of the NEW_RTX is modified by INSN. No way to forwarded >> + it any more. So add REG_EQUAL note to record its previous >> + value. This note can be used to check whether two RTXs >> + equal or not. */ >> + add_reg_note (insn, REG_EQUAL, copy_rtx (SET_SRC (set))); >> + } >> + >> validate_change (insn, loc, new_rtx, 1); >> return true; >> }