Message ID | 4C3D9C06.60901@codesourcery.com |
---|---|
State | New |
Headers | show |
On 07/14/10 05:14, Bernd Schmidt wrote: > When moving arguments into pseudos, we are being very careful not to > emit any instructions that could possibly clobber other argument > registers. Currently, we generate unnecessarily complicated sequences > of code for simple zero or sign extensions. > > With this patch, we check can_extend_p and the necessary predicates to > see if an extend insn is available for the conversion we have to do. Right, but what guarantee do we have that the conversion insn doesn't clobber a function argument register? ISTM that to be safe you actually have to scan the insns created by gen_extend_insn to ensure they don't clobber something important. I'm not an expert on what ports do these days, but I did work on a port (mn10200) where conversion "insns" where implemented as special function calls under the hood. I don't recall if we allowed those special function calls to have visible side effects, but if they did, they'd show up as clobbers/uses attached to the normal conversion insn. Of course the mn102 is dead, but I think it's method for implementing conversions was valid and if another port were to do something similar it would likely not interact well with your change. > If > so, we emit the insn directly, and create a REG_EQUIV note of the form > (sign_extend (mem)). Creating more REG_EQUIVs seems like a good idea to me. > Reload can't really do anything with these yet, > but there's an optimization in the register allocator to move argument > loads directly before their use if there's only one use. On Thumb-2 > this happens already, we seem to generate better code than before. > Unfortunately Thumb-1, which the PR is about, has some other problems > which I'll try to fix later. > In theory, given the REG_EQUIV note we ought to get an entry in reg_equiv_mem, which the code I'm working on knows it can use instead of shoving the pseudo into a stack slot. Effectively, my code will rematerialize the argument from the equivalent memory location regardless of the number of uses. Jeff
On 07/14/2010 08:54 PM, Jeff Law wrote: > On 07/14/10 05:14, Bernd Schmidt wrote: >> When moving arguments into pseudos, we are being very careful not to >> emit any instructions that could possibly clobber other argument >> registers. Currently, we generate unnecessarily complicated sequences >> of code for simple zero or sign extensions. >> >> With this patch, we check can_extend_p and the necessary predicates to >> see if an extend insn is available for the conversion we have to do. > Right, but what guarantee do we have that the conversion insn doesn't > clobber a function argument register? ISTM that to be safe you > actually have to scan the insns created by gen_extend_insn to ensure > they don't clobber something important. > > I'm not an expert on what ports do these days, but I did work on a port > (mn10200) where conversion "insns" where implemented as special function > calls under the hood. I don't recall if we allowed those special > function calls to have visible side effects, but if they did, they'd > show up as clobbers/uses attached to the normal conversion insn. Of > course the mn102 is dead, but I think it's method for implementing > conversions was valid and if another port were to do something similar > it would likely not interact well with your change. Hmm, ok. That's awful, but I kind of expected someone would say that. Did this really happen for integer zero/sign extend, or only for floating point stuff? If necessary I can try to test for a single insn with single_set and push it to the sequence otherwise. Bernd
On 07/14/2010 08:54 PM, Jeff Law wrote: > In theory, given the REG_EQUIV note we ought to get an entry in > reg_equiv_mem, which the code I'm working on knows it can use instead of > shoving the pseudo into a stack slot. Effectively, my code will > rematerialize the argument from the equivalent memory location > regardless of the number of uses. Reload can do that already, I think, but it's not prepared to handle a (zero_extend (mem)) inside a REG_EQUIV note. That should be reasonably trivial to add if the reg is never set other than in the initializing insn. Bernd
On 07/14/10 15:47, Bernd Schmidt wrote: > On 07/14/2010 08:54 PM, Jeff Law wrote: > >> In theory, given the REG_EQUIV note we ought to get an entry in >> reg_equiv_mem, which the code I'm working on knows it can use instead of >> shoving the pseudo into a stack slot. Effectively, my code will >> rematerialize the argument from the equivalent memory location >> regardless of the number of uses. >> > Reload can do that already, I think, but it's not prepared to handle a > (zero_extend (mem)) inside a REG_EQUIV note. That should be reasonably > trivial to add if the reg is never set other than in the initializing insn. > reload does, but it's code that can be problematical. For example, replacement of an unallocated pseudo with its equivalent can trigger secondary reloads, spills, etc. My code creates a new pseudo/allocno for the various ranges where the unallocated pseudo is live then asks IRA to color those split-range pseudos/allocnos. What we end up presenting to reload is cleaner; though often they result in the same final code. Handling this case falls out nicely from the infrastructure for splitting ranges of unallocated pseudos without equivalent forms (it's probably a couple dozen lines of new code). jeff
On 07/14/10 15:30, Bernd Schmidt wrote: > On 07/14/2010 08:54 PM, Jeff Law wrote: > >> On 07/14/10 05:14, Bernd Schmidt wrote: >> >>> When moving arguments into pseudos, we are being very careful not to >>> emit any instructions that could possibly clobber other argument >>> registers. Currently, we generate unnecessarily complicated sequences >>> of code for simple zero or sign extensions. >>> >>> With this patch, we check can_extend_p and the necessary predicates to >>> see if an extend insn is available for the conversion we have to do. >>> >> Right, but what guarantee do we have that the conversion insn doesn't >> clobber a function argument register? ISTM that to be safe you >> actually have to scan the insns created by gen_extend_insn to ensure >> they don't clobber something important. >> >> I'm not an expert on what ports do these days, but I did work on a port >> (mn10200) where conversion "insns" where implemented as special function >> calls under the hood. I don't recall if we allowed those special >> function calls to have visible side effects, but if they did, they'd >> show up as clobbers/uses attached to the normal conversion insn. Of >> course the mn102 is dead, but I think it's method for implementing >> conversions was valid and if another port were to do something similar >> it would likely not interact well with your change. >> > Hmm, ok. That's awful, but I kind of expected someone would say that. > Did this really happen for integer zero/sign extend, or only for > floating point stuff? > It was all the PSImode crap. > If necessary I can try to test for a single insn with single_set and > push it to the sequence otherwise. > For the mn103, the conversions were single insns... Ultimately, I think you have to peek at the insn(s) and see what registers they set/clobber. jeff
Index: function.c =================================================================== --- function.c (revision 162146) +++ function.c (working copy) @@ -2861,10 +2861,12 @@ static void assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm, struct assign_parm_data_one *data) { - rtx parmreg; + rtx parmreg, validated_mem; + rtx equiv_stack_parm; enum machine_mode promoted_nominal_mode; int unsignedp = TYPE_UNSIGNED (TREE_TYPE (parm)); bool did_conversion = false; + bool need_conversion, moved; /* Store the parm in a pseudoregister during the function, but we may need to do it in a wider mode. Using 2 here makes the result @@ -2891,10 +2893,46 @@ assign_parm_setup_reg (struct assign_par assign_parm_remove_parallels (data); + equiv_stack_parm = data->stack_parm; + validated_mem = validize_mem (data->entry_parm); + + need_conversion = (data->nominal_mode != data->passed_mode + || promoted_nominal_mode != data->promoted_mode); + moved = false; + /* Copy the value into the register, thus bridging between assign_parm_find_data_types and expand_expr_real_1. */ - if (data->nominal_mode != data->passed_mode - || promoted_nominal_mode != data->promoted_mode) + if (need_conversion) + { + enum insn_code icode; + rtx op0, op1; + + icode = can_extend_p (promoted_nominal_mode, data->passed_mode, + unsignedp); + + op0 = parmreg; + op1 = validated_mem; + if (icode != CODE_FOR_nothing + && insn_data[icode].operand[0].predicate (op0, promoted_nominal_mode) + && insn_data[icode].operand[1].predicate (op1, data->passed_mode)) + { + enum rtx_code code = unsignedp ? ZERO_EXTEND : SIGN_EXTEND; + rtx insn; + + insn = gen_extend_insn (op0, op1, promoted_nominal_mode, + data->passed_mode, unsignedp); + emit_insn (insn); + + equiv_stack_parm = gen_rtx_fmt_e (code, GET_MODE (parmreg), + equiv_stack_parm); + moved = true; + } + } + + if (moved) + /* Nothing to do. */ + ; + else if (need_conversion) { int save_tree_used; @@ -2919,7 +2957,7 @@ assign_parm_setup_reg (struct assign_par rtx tempreg = gen_reg_rtx (GET_MODE (data->entry_parm)); - emit_move_insn (tempreg, validize_mem (data->entry_parm)); + emit_move_insn (tempreg, validated_mem); push_to_sequence2 (all->first_conversion_insn, all->last_conversion_insn); tempreg = convert_to_mode (data->nominal_mode, tempreg, unsignedp); @@ -2949,7 +2987,7 @@ assign_parm_setup_reg (struct assign_par did_conversion = true; } else - emit_move_insn (parmreg, validize_mem (data->entry_parm)); + emit_move_insn (parmreg, validated_mem); /* If we were passed a pointer but the actual value can safely live in a register, put it in one. */ @@ -3034,7 +3072,7 @@ assign_parm_setup_reg (struct assign_par } else if ((set = single_set (linsn)) != 0 && SET_DEST (set) == parmreg) - set_unique_reg_note (linsn, REG_EQUIV, data->stack_parm); + set_unique_reg_note (linsn, REG_EQUIV, equiv_stack_parm); } /* For pointer data type, suggest pointer register. */