On 10/08/2014 08:31 AM, Jiong Wang wrote: > > On 30/09/14 19:36, Jiong Wang wrote: >> 2014-09-30 17:30 GMT+01:00 Jeff Law <l...@redhat.com>: >>> On 09/30/14 08:37, Jiong Wang wrote: >>>> >>>> On 30/09/14 05:21, Jeff Law wrote: >>>> >>>>> I do agree with Richard that it would be useful to see the insns that >>>>> are incorrectly sunk and the surrounding context. >>> So I must be missing something. I thought the shrink-wrapping code wouldn't >>> sink arithmetic/logical insns like we see with insn 14 and insn 182. I >>> thought it was limited to reg-reg copies and constant initializations. >> yes, it was limited to reg-reg copies, and my previous sink improvement >> aimed to >> sink any rtx >> >> A: be single_set >> B: the src operand be any combination of no more than one register >> and no non-constant objects. >> >> while some operator like shift may have side effect. IMHO, all side >> effects are reflected on RTX, >> together with this fail_on_clobber_use modification, the rtx returned >> by single_set_no_clobber_use is >> safe to sink if it meets the above limit B and pass later register >> use/def check in move_insn_for_shrink_wrap ? > > Ping ~ > > And as there is NONDEBUG_INSN_P check before move_insn_for_shrink_wrap > invoked, > we could avoid creating new wrapper function by invoke single_set_2 directly.
I'm committing the following to fix this. (1) Don't bother modifying single_set; just look for a bare SET. (2) Tighten the set of expressions we're willing to move. (3) Use direct "return false" in the failure case, rather than counting a non-zero number of non-constants, etc. Tested on x86_64, and against Andi's test case (unfortunately unreduced). r~
2014-10-10 Richard Henderson <r...@redhat.com> PR target/63404 * shrink-wrap.c (move_insn_for_shrink_wrap): Don't use single_set. Restrict the set of expressions we're willing to move. diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c index b1ff8a2..257812c 100644 --- a/gcc/shrink-wrap.c +++ b/gcc/shrink-wrap.c @@ -176,17 +176,40 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn, basic_block next_block; edge live_edge; - /* Look for a simple register copy. */ - set = single_set (insn); - if (!set) + /* Look for a simple register assignment. We don't use single_set here + because we can't deal with any CLOBBERs, USEs, or REG_UNUSED secondary + destinations. */ + if (!INSN_P (insn)) + return false; + set = PATTERN (insn); + if (GET_CODE (set) != SET) return false; src = SET_SRC (set); dest = SET_DEST (set); - if (!REG_P (src)) + /* For the destination, we want only a register. Also disallow STACK + or FRAME related adjustments. They are likely part of the prologue, + so keep them in the entry block. */ + if (!REG_P (dest) + || dest == stack_pointer_rtx + || dest == frame_pointer_rtx + || dest == hard_frame_pointer_rtx) + return false; + + /* For the source, we want one of: + (1) A (non-overlapping) register + (2) A constant, + (3) An expression involving no more than one register. + + That last point comes from the code following, which was originally + written to handle only register move operations, and still only handles + a single source register when checking for overlaps. Happily, the + same checks can be applied to expressions like (plus reg const). */ + + if (CONSTANT_P (src)) + ; + else if (!REG_P (src)) { - unsigned int reg_num = 0; - unsigned int nonconstobj_num = 0; rtx src_inner = NULL_RTX; if (can_throw_internal (insn)) @@ -196,30 +219,50 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn, FOR_EACH_SUBRTX_VAR (iter, array, src, ALL) { rtx x = *iter; - if (REG_P (x)) + switch (GET_RTX_CLASS (GET_CODE (x))) { - reg_num++; - src_inner = x; + case RTX_CONST_OBJ: + case RTX_COMPARE: + case RTX_COMM_COMPARE: + case RTX_BIN_ARITH: + case RTX_COMM_ARITH: + case RTX_UNARY: + case RTX_TERNARY: + /* Constant or expression. Continue. */ + break; + + case RTX_OBJ: + case RTX_EXTRA: + switch (GET_CODE (x)) + { + case UNSPEC: + case SUBREG: + case STRICT_LOW_PART: + case PC: + /* Ok. Continue. */ + break; + + case REG: + /* Fail if we see a second inner register. */ + if (src_inner != NULL) + return false; + src_inner = x; + break; + + default: + return false; + } + break; + + default: + return false; } - else if (!CONSTANT_P (x) && OBJECT_P (x)) - nonconstobj_num++; } - if (nonconstobj_num > 0 - || reg_num > 1) - src = NULL_RTX; - else if (reg_num == 1) + if (src_inner != NULL) src = src_inner; } - if (!REG_P (dest) || src == NULL_RTX - /* STACK or FRAME related adjustment might be part of prologue. - So keep them in the entry block. */ - || dest == stack_pointer_rtx - || dest == frame_pointer_rtx - || dest == hard_frame_pointer_rtx) - return false; - /* Make sure that the source register isn't defined later in BB. */ if (REG_P (src)) {