Message ID | 20111125184850.GQ27242@tyan-ft48-01.lab.bos.redhat.com |
---|---|
State | New |
Headers | show |
> Here is an attempt to make the check more complete (e.g. > the change wouldn't see overlap if addr was PLUS of two REGs, > where one of the REGs was based on internal_arg_pointer, etc.) > and less pessimistic. As tree-tailcall.c doesn't allow tail calls > from functions that have address of any of the caller's parameters > taken, IMHO it is enough to look for internal_arg_pointer based > pseudos initialized in the tail call sequence. > This patch scans the tail call sequence and notes which pseudos > are based on internal_arg_pointer (and what offset from > that pointer they have) and uses that in > mem_overlaps_already_clobbered_arg_p. This looks reasonable, but the logic is a bit hard to follow, especially the double usage of internal_arg_pointer_based_reg depending on SCAN's value. Would it be possible to split it into 2 functions that recursively call each other? You should also make it clearer that internal_arg_pointer_seq_start is part of the caching mechanism (I wondered for some minutes whether it has anything to do with RTL sequences), maybe: /* Last insn that has been scanned by internal_arg_pointer_based_reg, or NULL_RTX if none has been scanned yet. */ static rtx internal_arg_pointer_insn_cache; /* Vector indexed by REGNO - FIRST_PSEUDO_REGISTER, recording if a pseudo is based on crtl->args.internal_arg_pointer. The element is NULL_RTX if the pseudo isn't based on it, a CONST_INT offset if the pseudo is based on it with fixed offset, or PC if this is with variable or unknown offset. */ static VEC(rtx, heap) *internal_arg_pointer_pseudo_cache; > +/* Helper function for internal_arg_pointer_based_reg, called through > + for_each_rtx. Return 1 if a crtl->args.internal_arg_pointer based > + register is seen anywhere. */ > + > +static int > +internal_arg_pointer_based_reg_1 (rtx *loc, void *data ATTRIBUTE_UNUSED) > +{ > + if (REG_P (*loc) && internal_arg_pointer_based_reg (*loc, false) != > NULL_RTX) + return 1; > + if (MEM_P (*loc)) > + return -1; > + return 0; > +} Missing comment for the MEM_P case. > +/* If REG is based on crtl->args.internal_arg_pointer, return either > + a CONST_INT offset from crtl->args.internal_arg_pointer if > + offset from it is known constant, or PC if the offset is unknown. > + Return NULL_RTX if REG isn't based on crtl->args.internal_arg_pointer. > */ + > +static rtx > +internal_arg_pointer_based_reg (rtx reg, bool scan) > +{ > + rtx insn; > + > + if (CONSTANT_P (reg)) > + return NULL_RTX; > + > + if (reg == crtl->args.internal_arg_pointer) > + return const0_rtx; > + > + if (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER) > + return NULL_RTX; if (REG_P (reg) && HARD_REGISTER_P (reg)) return NULL_RTX;
On Mon, Nov 28, 2011 at 11:10:56PM +0100, Eric Botcazou wrote: > This looks reasonable, but the logic is a bit hard to follow, especially the > double usage of internal_arg_pointer_based_reg depending on SCAN's value. > Would it be possible to split it into 2 functions that recursively call each > other? Well, splitting off the scanning into separate function is quite easy, but that would still mean SCAN parameter. The thing is, for the common case where ADDR is already internal_arg_pointer, or sum of that and CONST_INT, or SYMBOL_REF, I'd prefer to avoid the scanning of the instructions and only do that if really needed (i.e. when either looking for a pseudo or arbitrary expression that needs to be for_each_rtx scanned). To get rid of the SCAN argument I'm afraid the first part of the function would need to be duplicated (for the current SCAN and !SCAN cases), then the scanning phase in another function and the last part in another one. > You should also make it clearer that internal_arg_pointer_seq_start is part of > the caching mechanism (I wondered for some minutes whether it has anything to > do with RTL sequences), maybe: Ok. > > +static int > > +internal_arg_pointer_based_reg_1 (rtx *loc, void *data ATTRIBUTE_UNUSED) > > +{ > > + if (REG_P (*loc) && internal_arg_pointer_based_reg (*loc, false) != > > NULL_RTX) + return 1; > > + if (MEM_P (*loc)) > > + return -1; > > + return 0; > > +} > > Missing comment for the MEM_P case. Will add. > > if (REG_P (reg) && HARD_REGISTER_P (reg)) > return NULL_RTX; Ok. BTW, it fixes also wrong-code PR51323, so I'll be including new testcase in the next patch iteration. Jakub
--- gcc/calls.c.jj 2011-11-08 23:35:12.000000000 +0100 +++ gcc/calls.c 2011-11-25 17:24:52.445878841 +0100 @@ -1658,6 +1658,106 @@ rtx_for_function_call (tree fndecl, tree return funexp; } +/* Last insn that has been already scanned by internal_arg_pointer_based_reg, + or NULL_RTX if none has been scanned yet. */ +static rtx internal_arg_pointer_seq_start; +/* Vector indexed by REGNO () - FIRST_PSEUDO_REGISTER, recoding if a pseudo + is based on crtl->args.internal_arg_pointer. It is NULL_RTX if not based + on it, some CONST_INT as offset from crtl->args.internal_arg_pointer + or PC for unknown offset from it. */ +static VEC(rtx, heap) *internal_arg_pointer_cache; + +static rtx internal_arg_pointer_based_reg (rtx, bool); + +/* Helper function for internal_arg_pointer_based_reg, called through + for_each_rtx. Return 1 if a crtl->args.internal_arg_pointer based + register is seen anywhere. */ + +static int +internal_arg_pointer_based_reg_1 (rtx *loc, void *data ATTRIBUTE_UNUSED) +{ + if (REG_P (*loc) && internal_arg_pointer_based_reg (*loc, false) != NULL_RTX) + return 1; + if (MEM_P (*loc)) + return -1; + return 0; +} + +/* If REG is based on crtl->args.internal_arg_pointer, return either + a CONST_INT offset from crtl->args.internal_arg_pointer if + offset from it is known constant, or PC if the offset is unknown. + Return NULL_RTX if REG isn't based on crtl->args.internal_arg_pointer. */ + +static rtx +internal_arg_pointer_based_reg (rtx reg, bool scan) +{ + rtx insn; + + if (CONSTANT_P (reg)) + return NULL_RTX; + + if (reg == crtl->args.internal_arg_pointer) + return const0_rtx; + + if (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER) + return NULL_RTX; + + if (GET_CODE (reg) == PLUS && CONST_INT_P (XEXP (reg, 1))) + { + rtx val = internal_arg_pointer_based_reg (XEXP (reg, 0), scan); + if (val == NULL_RTX || val == pc_rtx) + return val; + return plus_constant (val, INTVAL (XEXP (reg, 1))); + } + + if (!scan) + insn = NULL_RTX; + else if (internal_arg_pointer_seq_start == NULL_RTX) + insn = get_insns (); + else + insn = NEXT_INSN (internal_arg_pointer_seq_start); + while (insn) + { + rtx set = single_set (insn); + if (set + && REG_P (SET_DEST (set)) + && REGNO (SET_DEST (set)) >= FIRST_PSEUDO_REGISTER) + { + rtx val = NULL_RTX; + unsigned int idx = REGNO (SET_DEST (set)) - FIRST_PSEUDO_REGISTER; + /* Punt on pseudos set multiple times. */ + if (idx < VEC_length (rtx, internal_arg_pointer_cache) + && VEC_index (rtx, internal_arg_pointer_cache, idx) + != NULL_RTX) + val = pc_rtx; + else + val = internal_arg_pointer_based_reg (SET_SRC (set), false); + if (val != NULL_RTX) + { + VEC_safe_grow_cleared (rtx, heap, internal_arg_pointer_cache, + idx + 1); + VEC_replace (rtx, internal_arg_pointer_cache, idx, val); + } + } + if (NEXT_INSN (insn) == NULL_RTX) + internal_arg_pointer_seq_start = insn; + insn = NEXT_INSN (insn); + } + + if (REG_P (reg)) + { + unsigned int idx = REGNO (reg) - FIRST_PSEUDO_REGISTER; + if (idx < VEC_length (rtx, internal_arg_pointer_cache)) + return VEC_index (rtx, internal_arg_pointer_cache, idx); + else + return NULL_RTX; + } + + if (for_each_rtx (®, internal_arg_pointer_based_reg_1, NULL)) + return pc_rtx; + return NULL_RTX; +} + /* Return true if and only if SIZE storage units (usually bytes) starting from address ADDR overlap with already clobbered argument area. This function is used to determine if we should give up a @@ -1667,24 +1767,15 @@ static bool mem_overlaps_already_clobbered_arg_p (rtx addr, unsigned HOST_WIDE_INT size) { HOST_WIDE_INT i; + rtx val; - if (addr == crtl->args.internal_arg_pointer) - i = 0; - else if (GET_CODE (addr) == PLUS - && XEXP (addr, 0) == crtl->args.internal_arg_pointer - && CONST_INT_P (XEXP (addr, 1))) - i = INTVAL (XEXP (addr, 1)); - /* Return true for arg pointer based indexed addressing. */ - else if (GET_CODE (addr) == PLUS - && (XEXP (addr, 0) == crtl->args.internal_arg_pointer - || XEXP (addr, 1) == crtl->args.internal_arg_pointer)) - return true; - /* If the address comes in a register, we have no idea of its origin so - give up and conservatively return true. */ - else if (REG_P(addr)) + val = internal_arg_pointer_based_reg (addr, true); + if (val == NULL_RTX) + return false; + else if (val == pc_rtx) return true; else - return false; + i = INTVAL (val); #ifdef ARGS_GROW_DOWNWARD i = -i - size; @@ -3292,6 +3383,8 @@ expand_call (tree exp, rtx target, int i } sbitmap_free (stored_args_map); + internal_arg_pointer_seq_start = NULL_RTX; + VEC_free (rtx, heap, internal_arg_pointer_cache); } else {