| Message ID | CAFULd4a3KnrWnakDqMRiqtW7Aa-vv-Z3oGfTSK4CJuc39W2n5w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Uros Bizjak schrieb: > Hello! > > v2 patch differences: > - moves hook description text to target.def > - fixes error path to clear clobbers, as expected by recog_for_combine callers > > 2012-08-25 Uros Bizjak <ubizjak@gmail.com> > > * target.def (reject_combined_insn): New target hook. > * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. > * doc/tm.texi: Regenerated. > * combine.c (recog_for_combine): Call targetm.reject_combined_insn > to allow targets to reject combined insn. > > Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. > > OK for mainline? Just a question: avr.md could use this hook instead of the combine_pseudo_register_operand predicate in order to reject hard registers in some insns before register allocation. Is it valid to use INSN_CODE (insn)? E.g. compare it against CODE_FOR_foo for a light-wight detection of the insn in the backend? And may recod_data.operands[] et al. be used then? Or is garbage therein and the backend has to call recog on its own? The hook documentation could make this clear. In the case where the answer to the above questions is "no", i.e. the hook runs prior to recog: Why is this so? The performance gain is just minimal and sensible INSN_CODE and recog_data can make it considerably more easy to filter (u)nwanted insns instead of rewriting parts of recog in the hook. Johann
On Sat, 25 Aug 2012, Uros Bizjak wrote: > Hello! > > v2 patch differences: > - moves hook description text to target.def > - fixes error path to clear clobbers, as expected by recog_for_combine callers > > 2012-08-25 Uros Bizjak <ubizjak@gmail.com> > > * target.def (reject_combined_insn): New target hook. > * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. > * doc/tm.texi: Regenerated. > * combine.c (recog_for_combine): Call targetm.reject_combined_insn > to allow targets to reject combined insn. Maybe mention that the default is to allow all combinations for which a pattern match? And that the reason to disallow them can be that they're known to result in suboptimal code? Or other reasons. I don't think it should be mentioned that it can be used to stop invalid code to be generated; that'd just be encouraging covering up bugs in reload. (Using it to simplify the port may be valid.) brgds, H-P
On Sat, Aug 25, 2012 at 6:54 PM, Georg-Johann Lay <avr@gjlay.de> wrote: >> Hello! >> >> v2 patch differences: >> - moves hook description text to target.def >> - fixes error path to clear clobbers, as expected by recog_for_combine >> callers >> >> 2012-08-25 Uros Bizjak <ubizjak@gmail.com> >> >> * target.def (reject_combined_insn): New target hook. >> * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. >> * doc/tm.texi: Regenerated. >> * combine.c (recog_for_combine): Call targetm.reject_combined_insn >> to allow targets to reject combined insn. >> >> Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. >> >> OK for mainline? > > Just a question: > > avr.md could use this hook instead of the > combine_pseudo_register_operand predicate in order to reject > hard registers in some insns before register allocation. > Is it valid to use INSN_CODE (insn)? E.g. compare it against > CODE_FOR_foo for a light-wight detection of the insn in > the backend? No, you have to call recog (note, not recog_memoized) first to get new INSN_CODE. Note, that clobbers are added to the pattern in recog_for_combine just before the call to the hook. > And may recod_data.operands[] et al. be used then? No, you HAVE to set INSN_CODE of the tested insn and call extract_insn (insn) in the backend. > Or is garbage therein and the backend has to call recog on its own? There is garbage, backend has to do its own call to recog. > The hook documentation could make this clear. The hook in fact just receives combined insn, and recog_for_combine restores insn to original state before exit. Backend can use this insn as a test site for its (possibly destructive) checks. I agree that the documentation should say that backend does not need to restore insn to its original state. > In the case where the answer to the above questions is "no", > i.e. the hook runs prior to recog: Why is this so? Instruction with all its clobbers added to the pattern has not yet been fully recognized. Backed in free to re-recognize the insn to obtain its INSN_UID for checks or further processing. > The performance gain is just minimal and sensible INSN_CODE > and recog_data can make it considerably more easy to filter > (u)nwanted insns instead of rewriting parts of recog in the > hook. What you are referring to is x86 version of the hook that has to check for hard register constraints and check passed regs to these constraints. Where do you see reimplementation of recog? Uros.
On Sat, Aug 25, 2012 at 7:22 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: >> v2 patch differences: >> - moves hook description text to target.def >> - fixes error path to clear clobbers, as expected by recog_for_combine callers >> >> 2012-08-25 Uros Bizjak <ubizjak@gmail.com> >> >> * target.def (reject_combined_insn): New target hook. >> * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. >> * doc/tm.texi: Regenerated. >> * combine.c (recog_for_combine): Call targetm.reject_combined_insn >> to allow targets to reject combined insn. > > Maybe mention that the default is to allow all combinations for > which a pattern match? And that the reason to disallow them can > be that they're known to result in suboptimal code? Or other > reasons. Something like this perhaps: /* Returns true if the combined insn should be rejected for some reason. */ DEFHOOK (reject_combined_insn, "Take an instruction in @var{insn} and return @code{true} if the instruction\ should be rejected as a combination of two or more instructions. The\ default is to accept all instructions.", bool, (rtx insn), hook_bool_rtx_false) > I don't think it should be mentioned that it can be used to stop > invalid code to be generated; that'd just be encouraging > covering up bugs in reload. (Using it to simplify the port may > be valid.) Please note that there is no mention of the specific purpose of this hook. x86 uses it to counter a specific reload problem, but this is just hook _implementation_ detail. Thanks, Uros.
Uros Bizjak schrieb: > On Sat, Aug 25, 2012 at 6:54 PM, Georg-Johann Lay <avr@gjlay.de> wrote: > >>> Hello! >>> >>> v2 patch differences: >>> - moves hook description text to target.def >>> - fixes error path to clear clobbers, as expected by recog_for_combine >>> callers >>> >>> 2012-08-25 Uros Bizjak <ubizjak@gmail.com> >>> >>> * target.def (reject_combined_insn): New target hook. >>> * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. >>> * doc/tm.texi: Regenerated. >>> * combine.c (recog_for_combine): Call targetm.reject_combined_insn >>> to allow targets to reject combined insn. >>> >>> Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. >>> >>> OK for mainline? > >> Just a question: >> >> avr.md could use this hook instead of the >> combine_pseudo_register_operand predicate in order to reject >> hard registers in some insns before register allocation. > >> Is it valid to use INSN_CODE (insn)? E.g. compare it against >> CODE_FOR_foo for a light-wight detection of the insn in >> the backend? > > No, you have to call recog (note, not recog_memoized) first to get new > INSN_CODE. Note, that clobbers are added to the pattern in > recog_for_combine just before the call to the hook. > >> And may recod_data.operands[] et al. be used then? > > No, you HAVE to set INSN_CODE of the tested insn and call extract_insn > (insn) in the backend. > >> Or is garbage therein and the backend has to call recog on its own? > > There is garbage, backend has to do its own call to recog. > >> The hook documentation could make this clear. > > The hook in fact just receives combined insn, and recog_for_combine > restores insn to original state before exit. Backend can use this insn > as a test site for its (possibly destructive) checks. I agree that the > documentation should say that backend does not need to restore insn to > its original state. > >> In the case where the answer to the above questions is "no", >> i.e. the hook runs prior to recog: Why is this so? > > Instruction with all its clobbers added to the pattern has not yet > been fully recognized. Backed in free to re-recognize the insn to > obtain its INSN_UID for checks or further processing. > >> The performance gain is just minimal and sensible INSN_CODE >> and recog_data can make it considerably more easy to filter >> (u)nwanted insns instead of rewriting parts of recog in the >> hook. > > What you are referring to is x86 version of the hook that has to check > for hard register constraints and check passed regs to these > constraints. Where do you see reimplementation of recog? I am thinking of how this new hook could be used in the avr backend. For some reasons sign/zero source must not be a hard register at combine time, but combine propagates hard registers into the input operands. avr.md disallows that by a predicate: (define_insn "extendqisi2" [(set (match_operand:SI 0 "register_operand" "=r,r") (sign_extend:SI (match_operand:QI 1 "combine_pseudo_register_operand" "0,*r")))] With valid data from recog the avr version of the hook would be something like if (icode == CODE_FOR_extendqihi2 || icode == CODE_FOR_extendqipsi2 || icode == CODE_FOR_extendqisi2 || ...= { if (REGNO (recog_data[icode].operands[1]) < FIRST_PSEUDO_REGISTER) return true; } without you have to look into the insn anatomy: if (single_set (insn) && (SIGN_EXTEND == GET_CODE (SET_SRC (single_set (insn))) || ZERO_EXTEND == GET_CODE (SET_SRC (single_set (insn)))) && REG_P (XEXP (SET_SRC (single_set (insn)), 0)) && REGNO (XEXP (SET_SRC (single_set (insn)), 0)) < FIRST_PSEUDO_REGISTER) { return true; } for complex patterns this will turn to a recog-like XEXP orgy that is hard to read and maintain. Ugly. BTW: while I typed the lines above I find it more natural to return "true" for good patterns and "false" for bad patterns and not vice versa. But that's just a matter of taste. Johann
On Sat, 2012-08-25 at 20:25 +0200, Georg-Johann Lay wrote: > BTW: while I typed the lines above I find it more natural to > return "true" for good patterns and "false" for bad patterns > and not vice versa. But that's just a matter of taste. But then that won't fit with 'reject_combined_insn'. Reject? -> return 'true' for bad pattern that is to be rejected. Maybe if the hook was called something like 'legitimate_combined_insn'... Cheers, Oleg
Oleg Endo schrieb: > On Sat, 2012-08-25 at 20:25 +0200, Georg-Johann Lay wrote: > >> BTW: while I typed the lines above I find it more natural to >> return "true" for good patterns and "false" for bad patterns >> and not vice versa. But that's just a matter of taste. > > But then that won't fit with 'reject_combined_insn'. > Reject? -> return 'true' for bad pattern that is to be rejected. > Maybe if the hook was called something like > 'legitimate_combined_insn'... Ya, that would be a reasonable name and fit into already existing targetm.legitimate_foo namings. Johann
On Sat, Aug 25, 2012 at 11:21 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Sat, Aug 25, 2012 at 7:22 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > >>> v2 patch differences: >>> - moves hook description text to target.def >>> - fixes error path to clear clobbers, as expected by recog_for_combine callers >>> >>> 2012-08-25 Uros Bizjak <ubizjak@gmail.com> >>> >>> * target.def (reject_combined_insn): New target hook. >>> * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. >>> * doc/tm.texi: Regenerated. >>> * combine.c (recog_for_combine): Call targetm.reject_combined_insn >>> to allow targets to reject combined insn. >> >> Maybe mention that the default is to allow all combinations for >> which a pattern match? And that the reason to disallow them can >> be that they're known to result in suboptimal code? Or other >> reasons. > > Something like this perhaps: > > /* Returns true if the combined insn should be rejected > for some reason. */ > DEFHOOK > (reject_combined_insn, > "Take an instruction in @var{insn} and return @code{true} if the instruction\ > should be rejected as a combination of two or more instructions. The\ > default is to accept all instructions.", > bool, (rtx insn), > hook_bool_rtx_false) > >> I don't think it should be mentioned that it can be used to stop >> invalid code to be generated; that'd just be encouraging >> covering up bugs in reload. (Using it to simplify the port may >> be valid.) > > Please note that there is no mention of the specific purpose of this > hook. x86 uses it to counter a specific reload problem, but this is > just hook _implementation_ detail. What if we just add a global variable during_combine, and the insn predicates can check that variable? Then we wouldn't need a new target hook, one that seems slightly obscure to me. My apologies if this was already mentioned, I haven't followed the thread closely. Ian
Ian Lance Taylor schrieb: > On Sat, Aug 25, 2012 at 11:21 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Sat, Aug 25, 2012 at 7:22 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: >> >>>> v2 patch differences: >>>> - moves hook description text to target.def >>>> - fixes error path to clear clobbers, as expected by recog_for_combine callers >>>> >>>> 2012-08-25 Uros Bizjak <ubizjak@gmail.com> >>>> >>>> * target.def (reject_combined_insn): New target hook. >>>> * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. >>>> * doc/tm.texi: Regenerated. >>>> * combine.c (recog_for_combine): Call targetm.reject_combined_insn >>>> to allow targets to reject combined insn. >>> Maybe mention that the default is to allow all combinations for >>> which a pattern match? And that the reason to disallow them can >>> be that they're known to result in suboptimal code? Or other >>> reasons. >> Something like this perhaps: >> >> /* Returns true if the combined insn should be rejected >> for some reason. */ >> DEFHOOK >> (reject_combined_insn, >> "Take an instruction in @var{insn} and return @code{true} if the instruction\ >> should be rejected as a combination of two or more instructions. The\ >> default is to accept all instructions.", >> bool, (rtx insn), >> hook_bool_rtx_false) >> >>> I don't think it should be mentioned that it can be used to stop >>> invalid code to be generated; that'd just be encouraging >>> covering up bugs in reload. (Using it to simplify the port may >>> be valid.) >> Please note that there is no mention of the specific purpose of this >> hook. x86 uses it to counter a specific reload problem, but this is >> just hook _implementation_ detail. > > > What if we just add a global variable during_combine, and the insn > predicates can check that variable? Then we wouldn't need a new > target hook, one that seems slightly obscure to me. Instead of a global variable, more information on passes would be a good thing. Currently there is reload_completed or reload_in_progress, but sometimes similar information is needed for other passes like IRA, split1 or combine. A reasonable interface would be compiler->passes->combine->finished compiler->passes->split1->in_progress or similar, but currently such a thing. The only way is by comparing pass numbers; not nice. Johann > My apologies if this was already mentioned, I haven't followed the > thread closely. > > Ian
On Sat, 25 Aug 2012, Uros Bizjak wrote: > On Sat, Aug 25, 2012 at 7:22 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > > Maybe mention that the default is to allow all combinations for > > which a pattern match? And that the reason to disallow them can > > be that they're known to result in suboptimal code? Or other > > reasons. > > Something like this perhaps: > > /* Returns true if the combined insn should be rejected > for some reason. */ > DEFHOOK > (reject_combined_insn, > "Take an instruction in @var{insn} and return @code{true} if the instruction\ > should be rejected as a combination of two or more instructions. The\ > default is to accept all instructions.", > bool, (rtx insn), > hook_bool_rtx_false) Better. brgds, H-P
On Sat, Aug 25, 2012 at 9:40 PM, Ian Lance Taylor <iant@google.com> wrote: >>>> v2 patch differences: >>>> - moves hook description text to target.def >>>> - fixes error path to clear clobbers, as expected by recog_for_combine callers >>>> >>>> 2012-08-25 Uros Bizjak <ubizjak@gmail.com> >>>> >>>> * target.def (reject_combined_insn): New target hook. >>>> * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. >>>> * doc/tm.texi: Regenerated. >>>> * combine.c (recog_for_combine): Call targetm.reject_combined_insn >>>> to allow targets to reject combined insn. >>> >>> Maybe mention that the default is to allow all combinations for >>> which a pattern match? And that the reason to disallow them can >>> be that they're known to result in suboptimal code? Or other >>> reasons. >> >> Something like this perhaps: >> >> /* Returns true if the combined insn should be rejected >> for some reason. */ >> DEFHOOK >> (reject_combined_insn, >> "Take an instruction in @var{insn} and return @code{true} if the instruction\ >> should be rejected as a combination of two or more instructions. The\ >> default is to accept all instructions.", >> bool, (rtx insn), >> hook_bool_rtx_false) >> >>> I don't think it should be mentioned that it can be used to stop >>> invalid code to be generated; that'd just be encouraging >>> covering up bugs in reload. (Using it to simplify the port may >>> be valid.) >> >> Please note that there is no mention of the specific purpose of this >> hook. x86 uses it to counter a specific reload problem, but this is >> just hook _implementation_ detail. > > > What if we just add a global variable during_combine, and the insn > predicates can check that variable? Then we wouldn't need a new > target hook, one that seems slightly obscure to me. No, in x86 case we need to look at operand constraints of the insn, and we need to recognize and analyse the insn. This way, we can avoid i.e. "ax_reg", "nonimm_ax_reg" and such register constraints. Uros.
On Sun, Aug 26, 2012 at 8:11 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> What if we just add a global variable during_combine, and the insn >> predicates can check that variable? Then we wouldn't need a new >> target hook, one that seems slightly obscure to me. > > No, in x86 case we need to look at operand constraints of the insn, > and we need to recognize and analyse the insn. This way, we can avoid > i.e. "ax_reg", "nonimm_ax_reg" and such register constraints. Please also note that with "enabled" attribute, operand constraints are dynamically changed, so predicate would need to track these changes (i.e. "*umul<mode><dwi>3_1"). BTW: I found the proposed "legitimate_combined_insn" name much more descriptive, I will update the patch accordingly. Uros.
Index: target.def =================================================================== --- target.def (revision 190665) +++ target.def (working copy) @@ -1984,7 +1984,16 @@ DEFHOOK const char *, (const_rtx insn), default_invalid_within_doloop) +/* Returns true if the combined insn should be rejected + for some reason. */ DEFHOOK +(reject_combined_insn, +"Take an instruction in @var{insn} and return @code{true} if the instruction\ + should be rejected as a combination of two or more instructions.", + bool, (rtx insn), + hook_bool_rtx_false) + +DEFHOOK (valid_dllimport_attribute_p, "@var{decl} is a variable or function with @code{__attribute__((dllimport))}\ specified. Use this hook if the target needs to add extra validation\ Index: combine.c =================================================================== --- combine.c (revision 190665) +++ combine.c (working copy) @@ -10500,11 +10500,13 @@ static int recog_for_combine (rtx *pnewpat, rtx insn, rtx *pnotes) { rtx pat = *pnewpat; + rtx pat_without_clobbers; int insn_code_number; int num_clobbers_to_add = 0; int i; - rtx notes = 0; + rtx notes = NULL_RTX; rtx old_notes, old_pat; + int old_icode; /* If PAT is a PARALLEL, check to see if it contains the CLOBBER we use to indicate that something didn't match. If we find such a @@ -10518,7 +10520,7 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn old_pat = PATTERN (insn); old_notes = REG_NOTES (insn); PATTERN (insn) = pat; - REG_NOTES (insn) = 0; + REG_NOTES (insn) = NULL_RTX; insn_code_number = recog (pat, insn, &num_clobbers_to_add); if (dump_file && (dump_flags & TDF_DETAILS)) @@ -10564,6 +10566,9 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn print_rtl_single (dump_file, pat); } } + + pat_without_clobbers = pat; + PATTERN (insn) = old_pat; REG_NOTES (insn) = old_notes; @@ -10605,6 +10610,35 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn pat = newpat; } + if (insn_code_number >= 0 + && insn_code_number != NOOP_MOVE_INSN_CODE) + { + old_pat = PATTERN (insn); + old_notes = REG_NOTES (insn); + old_icode = INSN_CODE (insn); + PATTERN (insn) = pat; + REG_NOTES (insn) = notes; + + /* Allow targets to reject combined insn. */ + if (targetm.reject_combined_insn (insn)) + { + if (dump_file && (dump_flags & TDF_DETAILS)) + fputs ("Instruction not appropriate for target.", + dump_file); + + /* Callers expect recog_for_combine to strip + clobbers from the pattern on failure. */ + pat = pat_without_clobbers; + notes = NULL_RTX; + + insn_code_number = -1; + } + + PATTERN (insn) = old_pat; + REG_NOTES (insn) = old_notes; + INSN_CODE (insn) = old_icode; + } + *pnewpat = pat; *pnotes = notes; Index: doc/tm.texi =================================================================== --- doc/tm.texi (revision 190665) +++ doc/tm.texi (working copy) @@ -10938,6 +10938,10 @@ By default, the RTL loop optimizer does not use a loops containing function calls or branch on table instructions. @end deftypefn +@deftypefn {Target Hook} bool TARGET_REJECT_COMBINED_INSN (rtx @var{insn}) +Take an instruction in @var{insn} and return @code{true} if the instruction should be rejected as a combination of two or more instructions. +@end deftypefn + @defmac MD_CAN_REDIRECT_BRANCH (@var{branch1}, @var{branch2}) Take a branch insn in @var{branch1} and another in @var{branch2}. Index: doc/tm.texi.in =================================================================== --- doc/tm.texi.in (revision 190665) +++ doc/tm.texi.in (working copy) @@ -10796,6 +10796,8 @@ By default, the RTL loop optimizer does not use a loops containing function calls or branch on table instructions. @end deftypefn +@hook TARGET_REJECT_COMBINED_INSN + @defmac MD_CAN_REDIRECT_BRANCH (@var{branch1}, @var{branch2}) Take a branch insn in @var{branch1} and another in @var{branch2}.
Hello! v2 patch differences: - moves hook description text to target.def - fixes error path to clear clobbers, as expected by recog_for_combine callers 2012-08-25 Uros Bizjak <ubizjak@gmail.com> * target.def (reject_combined_insn): New target hook. * doc/tm.texi.in (TARGET_REJECT_COMBINED_INSN): New hook. * doc/tm.texi: Regenerated. * combine.c (recog_for_combine): Call targetm.reject_combined_insn to allow targets to reject combined insn. Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline? Uros.