| Message ID | CAFULd4Y2NZXtpiyqLFWJc1r-H7qmrN7Gtb6q99V0P_FNXjyd5w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Uros Bizjak wrote: > Hello! > > This patch introduces TARGET_REJECT_COMBINED_INSN target hook, so > targets are able to reject combinations of two or more insns. The hook > is called from recog_for_combine, so it is the target that has the > final say on the combined insn. Hi, great place for a hook, it was missing, IMO. Just a note: Wouldn't it be good to have a hook that may transform a pattern to a new one and return that to combine? Your reject_combined_insn would be a special case, e.g. return NULL_RTX. Sometimes recog_for_combine fails (resp. recog fails) because recog_for_combine does not try all possible transformations and therefore recog then fails because there is no combine pattern. However, there may be a pattern that does exactly the same thing but is written down differently. The backend then could try to canonicalize the pattern before it goes into recog. An other question is: I always wondered if it is possible to transform code like (set (reg:SI 0) (ior:SI (reg SI:1) (reg SI:2))) or more complicated, combined patterns to a different pattern if there is some additional knowledge. For example, a backend may have an insn that can do the combined operation efficiently, but only if reg2 is a boolean value (0 or 1). Currently you will have to write a combine pattern like (set (reg:SI 0) (ior:SI (reg SI:1) (and:SI (reg SI:2) (const_int 0)))) and/or (set (reg:SI 0) (ior:SI (reg SI:1) (zero_extract:SI (reg:SI 2) (const_int 1) (const_int x))))) and/or (set (reg:SI 0) (ior:SI (reg SI:1) (and:SI (lshiftrt:SI (reg SI:2) (const_int x)) (const_int 1)))) You can imagine that it is really tedious to write down all these patterns and describe their rtx_costs. If the targed had a way to say "this transformation is okay under the condition X" that would be great. combine collects many information on the values like ranges and set/unset bits, but that information cannot be used for matching/rejecting on the insn level, e.g. in an insn predicate or insn condition. A Hook would do this: - If the pattern is to be rejected, return NULL_RTX. - If the pattern is fine, return it (recog will run on it). - If the target can make use of additional information, it might return the origonal pattern or reject it. Here again, recog runs on the pattern (provided the hook returned non-NULL). - The hook may canonicalize the pattern and cook a new one. In that case the backend is responsible for a correct transformation. Also in this case recog runs on the returned pattern. Another use case could be to fold away an UNSPEC. From the hook perspective, it's no additional work: Just call the hook from recog_for_combine, receive the pattern back from the backend, check for NULL_RTX and then run recog. Thanks, Johann > This target hook will be used in a follow-up x86 patch that rejects > instructions where hard registers don't fit into operand register > constraint. > > 2012-08-23 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. > > OK for mainline? > > Uros.
On Thu, Aug 23, 2012 at 10:52 AM, Georg-Johann Lay <avr@gjlay.de> wrote: > Uros Bizjak wrote: >> Hello! >> >> This patch introduces TARGET_REJECT_COMBINED_INSN target hook, so >> targets are able to reject combinations of two or more insns. The hook >> is called from recog_for_combine, so it is the target that has the >> final say on the combined insn. > > Hi, > > great place for a hook, it was missing, IMO. > > Just a note: Wouldn't it be good to have a hook that may transform > a pattern to a new one and return that to combine? > > Your reject_combined_insn would be a special case, e.g. return > NULL_RTX. > > Sometimes recog_for_combine fails (resp. recog fails) because > recog_for_combine does not try all possible transformations and > therefore recog then fails because there is no combine pattern. Or just better yet improve recog_for_combine. I was planing on doing that for some cases dealing with zero_extract. Thanks, Andrew Pinski > > However, there may be a pattern that does exactly the same thing > but is written down differently. The backend then could try to > canonicalize the pattern before it goes into recog. > > An other question is: > > I always wondered if it is possible to transform code like > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (reg SI:2))) > > or more complicated, combined patterns to a different pattern > if there is some additional knowledge. > > For example, a backend may have an insn that can do the > combined operation efficiently, but only if reg2 is a > boolean value (0 or 1). > > Currently you will have to write a combine pattern like > > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (and:SI (reg SI:2) > (const_int 0)))) > > and/or > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (zero_extract:SI (reg:SI 2) > (const_int 1) > (const_int x))))) > > and/or > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (and:SI (lshiftrt:SI (reg SI:2) > (const_int x)) > (const_int 1)))) > > You can imagine that it is really tedious to write > down all these patterns and describe their rtx_costs. > > If the targed had a way to say "this transformation is > okay under the condition X" that would be great. > > combine collects many information on the values like > ranges and set/unset bits, but that information cannot > be used for matching/rejecting on the insn level, > e.g. in an insn predicate or insn condition. > > A Hook would do this: > > - If the pattern is to be rejected, return NULL_RTX. > > - If the pattern is fine, return it (recog will run on it). > > - If the target can make use of additional information, > it might return the origonal pattern or reject it. > Here again, recog runs on the pattern (provided the hook > returned non-NULL). > > - The hook may canonicalize the pattern and cook a new one. > In that case the backend is responsible for a correct > transformation. Also in this case recog runs on the > returned pattern. > > Another use case could be to fold away an UNSPEC. > > From the hook perspective, it's no additional work: > Just call the hook from recog_for_combine, receive the > pattern back from the backend, check for NULL_RTX and > then run recog. > > Thanks, > > Johann > > >> This target hook will be used in a follow-up x86 patch that rejects >> instructions where hard registers don't fit into operand register >> constraint. >> >> 2012-08-23 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. >> >> OK for mainline? >> >> Uros. >
On Thu, Aug 23, 2012 at 7:59 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>> This patch introduces TARGET_REJECT_COMBINED_INSN target hook, so >>> targets are able to reject combinations of two or more insns. The hook >>> is called from recog_for_combine, so it is the target that has the >>> final say on the combined insn. >> >> Hi, >> >> great place for a hook, it was missing, IMO. >> >> Just a note: Wouldn't it be good to have a hook that may transform >> a pattern to a new one and return that to combine? >> >> Your reject_combined_insn would be a special case, e.g. return >> NULL_RTX. >> >> Sometimes recog_for_combine fails (resp. recog fails) because >> recog_for_combine does not try all possible transformations and >> therefore recog then fails because there is no combine pattern. > > Or just better yet improve recog_for_combine. I was planing on doing > that for some cases dealing with zero_extract. We have tried that (see previous discussions on recog_for_combine change. But there is no way to satisfy all targets: some works better when reload fixes invalid hard regs (sh), others (x86) require totally different strategy. Even if recog_for_combine will be improved for all targets, we will still need something target specific. Uros.
On Thu, Aug 23, 2012 at 7:52 PM, Georg-Johann Lay <avr@gjlay.de> wrote: >> This patch introduces TARGET_REJECT_COMBINED_INSN target hook, so >> targets are able to reject combinations of two or more insns. The hook >> is called from recog_for_combine, so it is the target that has the >> final say on the combined insn. > > great place for a hook, it was missing, IMO. > > Just a note: Wouldn't it be good to have a hook that may transform > a pattern to a new one and return that to combine? > > Your reject_combined_insn would be a special case, e.g. return > NULL_RTX. > > Sometimes recog_for_combine fails (resp. recog fails) because > recog_for_combine does not try all possible transformations and > therefore recog then fails because there is no combine pattern. > > However, there may be a pattern that does exactly the same thing > but is written down differently. The backend then could try to > canonicalize the pattern before it goes into recog. This is the job for simplify_rtx, it should canonicalize insn into some "standard" form. > > An other question is: > > I always wondered if it is possible to transform code like > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (reg SI:2))) > > or more complicated, combined patterns to a different pattern > if there is some additional knowledge. > > For example, a backend may have an insn that can do the > combined operation efficiently, but only if reg2 is a > boolean value (0 or 1). > > Currently you will have to write a combine pattern like > > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (and:SI (reg SI:2) > (const_int 0)))) > > and/or > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (zero_extract:SI (reg:SI 2) > (const_int 1) > (const_int x))))) > > and/or > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (and:SI (lshiftrt:SI (reg SI:2) > (const_int x)) > (const_int 1)))) > > You can imagine that it is really tedious to write > down all these patterns and describe their rtx_costs. > > If the targed had a way to say "this transformation is > okay under the condition X" that would be great. > > combine collects many information on the values like > ranges and set/unset bits, but that information cannot > be used for matching/rejecting on the insn level, > e.g. in an insn predicate or insn condition. > > A Hook would do this: > > - If the pattern is to be rejected, return NULL_RTX. > > - If the pattern is fine, return it (recog will run on it). > > - If the target can make use of additional information, > it might return the origonal pattern or reject it. > Here again, recog runs on the pattern (provided the hook > returned non-NULL). > > - The hook may canonicalize the pattern and cook a new one. > In that case the backend is responsible for a correct > transformation. Also in this case recog runs on the > returned pattern. I believe that this is the job for combine_simplify_rtx, so the combined RTX gets simplified to some standard form. Your example above has its counterpart in x86 zero-extended addresses, where we have a nice collection of SUBREGs, ANDs and ZERO_EXTENDs in all possible combinations. > Another use case could be to fold away an UNSPEC. No, I don't agree here. I'd recommend to avoid UNSPECs as much as possible, and a precise description of the insn should be used from the beginning. > From the hook perspective, it's no additional work: > Just call the hook from recog_for_combine, receive the > pattern back from the backend, check for NULL_RTX and > then run recog. Your proposed simplifications should be implemented as standard RTX simplifications, but given the fact than nobody bothered with it suggest that they are not that critical. From my experience, two or maybe three patterns fits all cases. Uros.
Hello, On Thu, 2012-08-23 at 19:52 +0200, Georg-Johann Lay wrote: > Uros Bizjak wrote: > > Hello! > > > > This patch introduces TARGET_REJECT_COMBINED_INSN target hook, so > > targets are able to reject combinations of two or more insns. The hook > > is called from recog_for_combine, so it is the target that has the > > final say on the combined insn. > > Hi, > > great place for a hook, it was missing, IMO. > > Just a note: Wouldn't it be good to have a hook that may transform > a pattern to a new one and return that to combine? As far as I understand that's what the splits during combine are for. However, I always get surprised when combine would actually take the split and continue trying combinations with the insns that came out from the split, and when it won't. Sometimes it works, sometimes it doesn't. Very often I have to resort to insn_and_split, which is similar, but not the same ... > However, there may be a pattern that does exactly the same thing > but is written down differently. The backend then could try to > canonicalize the pattern before it goes into recog. Do you mean something like already existing CANONICALIZE_COMPARISON macro? If so, then probably one would have to match/transform the patterns in C, which is not 'very nice', IMHO. Maybe relaxing the combine-splitting (not insn_and_split, just define_split) would be better for this? In any case, if combine doesn't try out all the combinations, then something would have to do that in the backend and everything is back to square one, except that every backend (sooner or later) will have some kind of half-combine-half-recog implementation in it, won't it? > > An other question is: > > I always wondered if it is possible to transform code like > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (reg SI:2))) > > or more complicated, combined patterns to a different pattern > if there is some additional knowledge. > > For example, a backend may have an insn that can do the > combined operation efficiently, but only if reg2 is a > boolean value (0 or 1). > > Currently you will have to write a combine pattern like > > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (and:SI (reg SI:2) > (const_int 0)))) > > and/or > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (zero_extract:SI (reg:SI 2) > (const_int 1) > (const_int x))))) > > and/or > > (set (reg:SI 0) > (ior:SI (reg SI:1) > (and:SI (lshiftrt:SI (reg SI:2) > (const_int x)) > (const_int 1)))) > > You can imagine that it is really tedious to write > down all these patterns and describe their rtx_costs. I think in this case one could define a predicate for those and then re-use it in multiple patterns. As for the costs, it would really be nice if it was possible to just say: "If this pattern is matched, the total cost is X", instead of partially re-implementing/describing the patterns in C in the rtx_costs function. I was already thinking of using recog functions in the rtx_costs function, but I'm not sure whether it is the right thing to do. I think one of the prime examples that would benefit from those things are patterns to implement instructions such as bit-tests with constants (on SH: tst #imm8,R0), where the basic pattern is simply: (define_insn "tstsi_t" [(set (reg:SI T_REG) (eq:SI (and:SI (match_operand:SI 0 "logical_operand" "%z,r") (match_operand:SI 1 "logical_operand" "K08,r")) (const_int 0)))] .. but then 9 variations are required to make it really work under various circumstances. Cheers, Oleg
On Thu, 23 Aug 2012, Uros Bizjak wrote: > 2012-08-23 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. The preferred location for hook documentation is in the doc string in target.def rather than in tm.texi.in. (If you wish to move *existing* text from tm.texi.in to such a doc string in target.def, CC me or Diego or Gerald on the patch and ask for docstring relicensing review.)
Oleg Endo schrieb: > Hello, > > On Thu, 2012-08-23 at 19:52 +0200, Georg-Johann Lay wrote: >> Uros Bizjak wrote: >>> Hello! >>> >>> This patch introduces TARGET_REJECT_COMBINED_INSN target hook, so >>> targets are able to reject combinations of two or more insns. The hook >>> is called from recog_for_combine, so it is the target that has the >>> final say on the combined insn. >> Hi, >> >> great place for a hook, it was missing, IMO. >> >> Just a note: Wouldn't it be good to have a hook that may transform >> a pattern to a new one and return that to combine? > > As far as I understand that's what the splits during combine are for. These patters are very specific. They only can map one set to two sets. It's not possible to implement a canonicalization for anything combine might come up with and that is just ready to be stuffed into recog. > However, I always get surprised when combine would actually take the > split and continue trying combinations with the insns that came out from > the split, and when it won't. Sometimes it works, sometimes it doesn't. > Very often I have to resort to insn_and_split, which is similar, but not > the same ... The advantage is that you can da any transform in split1, even with new pseudos. The disadvantage is that the result does not get back into combine and that in split1 the meta-information om involved data is lost (zeroed bits etc.). At least that information is not available in insn condition or predicate. >> However, there may be a pattern that does exactly the same thing >> but is written down differently. The backend then could try to >> canonicalize the pattern before it goes into recog. > > Do you mean something like already existing CANONICALIZE_COMPARISON > macro? If so, then probably one would have to match/transform the > patterns in C, which is not 'very nice', IMHO. Maybe relaxing the Yes, it's a hook and in C. Sometime C can be more convenient. For the hook in question, it would be the same effort as far as the hook is concerned: Ir really makes no difference if you - Pass X to the hook and return true or false - Pass X to the hook and return X or NULL_RTX. However, the latter interface is much more general and powerful and allows to change X -- or simply leave it alone like in legitimize_address (target_legitimize_combine or so). Some years ago there were plans for a define_combine thing for the backend (similar to define_insn/split/expand/peephole etc.) But that idea was dropped for some reason or in favor of a better or easier approach. > combine-splitting (not insn_and_split, just define_split) would be > better for this? RTL is great. I think it makes reading and writing back ends much more robust, easy and powerful. And I always wondered why there is no tree/SSA equivalent of RTL. However, sometimes you are better off with C. > In any case, if combine doesn't try out all the combinations, then > something would have to do that in the backend and everything is back to > square one, except that every backend (sooner or later) will have some > kind of half-combine-half-recog implementation in it, won't it? Yes, you are right. There is already too much recog-like code in the backends, for example in rtx_costs. In a private port I call recog from rtx_costs to get the costs from insn attributes. Works very nice and is much better to maintain than XEXP for many, many insns and patterns... >> An other question is: >> >> I always wondered if it is possible to transform code like >> >> (set (reg:SI 0) >> (ior:SI (reg SI:1) >> (reg SI:2))) >> >> or more complicated, combined patterns to a different pattern >> if there is some additional knowledge. >> >> For example, a backend may have an insn that can do the >> combined operation efficiently, but only if reg2 is a >> boolean value (0 or 1). >> >> Currently you will have to write a combine pattern like >> >> (set (reg:SI 0) >> (ior:SI (reg SI:1) >> (and:SI (reg SI:2) >> (const_int 0)))) >> >> and/or >> >> (set (reg:SI 0) >> (ior:SI (reg SI:1) >> (zero_extract:SI (reg:SI 2) >> (const_int 1) >> (const_int x))))) >> >> and/or >> >> (set (reg:SI 0) >> (ior:SI (reg SI:1) >> (and:SI (lshiftrt:SI (reg SI:2) >> (const_int x)) >> (const_int 1)))) >> >> You can imagine that it is really tedious to write >> down all these patterns and describe their rtx_costs. > > I think in this case one could define a predicate for those and then > re-use it in multiple patterns. Been there, done it: http://gcc.gnu.org/ml/gcc/2010-11/msg00222.html This needed yet another hook in find_reloads because you don't want to reload the whole predicate; you want to reload the individual operands. Therefore this needs a mapping from the predicate to its operands. Currently, reload has only magic for unary rtxes like zero_extract. > As for the costs, it would really be nice if it was possible to just > say: "If this pattern is matched, the total cost is X", instead of > partially re-implementing/describing the patterns in C in the rtx_costs > function. I was already thinking of using recog functions in the > rtx_costs function, but I'm not sure whether it is the right thing to > do. Yes. Just call recog in combine :-) It needs some preparation, but "proof of concept" work is already done and works smooth in 45 and 4.6, and I don't see a reason why it should not work in newer versions. If you are interested I can explain the caveats and details. Johann > I think one of the prime examples that would benefit from those things > are patterns to implement instructions such as bit-tests with constants > (on SH: tst #imm8,R0), where the basic pattern is simply: > > (define_insn "tstsi_t" > [(set (reg:SI T_REG) > (eq:SI (and:SI (match_operand:SI 0 "logical_operand" "%z,r") > (match_operand:SI 1 "logical_operand" "K08,r")) > (const_int 0)))] > > .. but then 9 variations are required to make it really work under > various circumstances. > > Cheers, > Oleg
On Thu, 2012-08-23 at 22:46 +0200, Georg-Johann Lay wrote: > > > However, I always get surprised when combine would actually take the > > split and continue trying combinations with the insns that came out from > > the split, and when it won't. Sometimes it works, sometimes it doesn't. > > Very often I have to resort to insn_and_split, which is similar, but not > > the same ... > > The advantage is that you can da any transform in split1, even with new > pseudos. The disadvantage is that the result does not get back into > combine and that in split1 the meta-information om involved data is > lost (zeroed bits etc.). At least that information is not available > in insn condition or predicate. Yes, that's what I meant by 'which is similar, but not the same ...' > RTL is great. I think it makes reading and writing back ends much > more robust, easy and powerful. And I always wondered why there is > no tree/SSA equivalent of RTL. > > However, sometimes you are better off with C. True. > Yes, you are right. There is already too much recog-like code in the > backends, for example in rtx_costs. In a private port I call recog > from rtx_costs to get the costs from insn attributes. Works very nice > and is much better to maintain than XEXP for many, many insns and > patterns... Yep, that's what I meant by 'have to match/transform the patterns in C, which is not very nice'. > Been there, done it: http://gcc.gnu.org/ml/gcc/2010-11/msg00222.html > This needed yet another hook in find_reloads because you don't want > to reload the whole predicate; you want to reload the individual > operands. Therefore this needs a mapping from the predicate to its > operands. Currently, reload has only magic for unary rtxes like > zero_extract. I see. Thanks for the pointers. > Yes. Just call recog in combine :-) It needs some preparation, but > "proof of concept" work is already done and works smooth in 45 and 4.6, > and I don't see a reason why it should not work in newer versions. > If you are interested I can explain the caveats and details. Yes, I am very much interested indeed. Please do carry on (either on the list or in private) :) Wouldn't it somehow make sense to establish a 'standard' way of doing this kind of thing (getting insn costs from attributes) for all backends? As an option of course, not requiring every backend to do it that way from day one. Cheers, Oleg
On Thu, Aug 23, 2012 at 10:46 PM, Georg-Johann Lay <avr@gjlay.de> wrote: > For the hook in question, it would be the same effort as far as > the hook is concerned: Ir really makes no difference if you > > - Pass X to the hook and return true or false > > - Pass X to the hook and return X or NULL_RTX. > > However, the latter interface is much more general and powerful and > allows to change X -- or simply leave it alone like in > legitimize_address (target_legitimize_combine or so). I did some experiments with your proposal, but it is not as simple as it is written above. We use existing insn as a testing place, where we stuff various combinations and call recog. After all processing, we have to restore insn to its original state and singal recog_for_combine caller that we recognized the combination, but with optional clobbers. So, calling sites only expect the confirmation/rejection of the proposed patterns, and in case of confirmation, recog_for_combine is allowed to decorate the pattern with optional clobbers. Your proposal that recog_for_combine changes the pattern is not what callers expect from this function. Probably, there are better places to implement target-dependent transformations, and leave recog_for_combine just the task to confirm that them combined pattern is OK. Uros.
On Thu, Aug 23, 2012 at 8:59 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: >> 2012-08-23 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. > > The preferred location for hook documentation is in the doc string in > target.def rather than in tm.texi.in. (If you wish to move *existing* > text from tm.texi.in to such a doc string in target.def, CC me or Diego or > Gerald on the patch and ask for docstring relicensing review.) No problem, will put the string in target.def (I really wondered what the empty quotes were for). Thanks, Uros.
Uros Bizjak schrieb: > On Thu, Aug 23, 2012 at 10:46 PM, Georg-Johann Lay <avr@gjlay.de> wrote: > >> For the hook in question, it would be the same effort as far as >> the hook is concerned: Ir really makes no difference if you >> >> - Pass X to the hook and return true or false >> >> - Pass X to the hook and return X or NULL_RTX. >> >> However, the latter interface is much more general and powerful and >> allows to change X -- or simply leave it alone like in >> legitimize_address (target_legitimize_combine or so). > > I did some experiments with your proposal, but it is not as simple as > it is written above. We use existing insn as a testing place, where we > stuff various combinations and call recog. After all processing, we > have to restore insn to its original state and singal > recog_for_combine caller that we recognized the combination, but with > optional clobbers. So, calling sites only expect the > confirmation/rejection of the proposed patterns, and in case of > confirmation, recog_for_combine is allowed to decorate the pattern > with optional clobbers. If I understand you correctly, problems would arise if such a hook produced a pattern that is *not* recognized. In such a case combine performs a roll-back, and with a changed pattern some assumptions will break? Are there also problems if the hook transformed to a pattern that is known to be recognized? Like "I know this pattern, just canonicalize it to an expression that will match". In that case no roll-back is needed. > Your proposal that recog_for_combine changes the pattern is not what > callers expect from this function. Probably, there are better places > to implement target-dependent transformations, and leave > recog_for_combine just the task to confirm that them combined pattern > is OK. Yes in an ideal world such a hook/transformation would not be needed and combine could perform all the magic. combine is a very powerful pass to cook up new instructions out of the blue, but unfortunately there are some shortcomings that cannot be worked around in a backend. One example is the problem of intermediate insn to reach a complex insn. Another problem is that combine does not try everything. It's greedy to generate and try PARALLELs, but for ordinary arithmetic it misses to cook up valid combination even if they are within combine's limited complexity. Johann
On Sat, Aug 25, 2012 at 11:55 AM, Georg-Johann Lay <avr@gjlay.de> wrote: >>> For the hook in question, it would be the same effort as far as >>> the hook is concerned: Ir really makes no difference if you >>> >>> - Pass X to the hook and return true or false >>> >>> - Pass X to the hook and return X or NULL_RTX. >>> >>> However, the latter interface is much more general and powerful and >>> allows to change X -- or simply leave it alone like in >>> legitimize_address (target_legitimize_combine or so). >> >> >> I did some experiments with your proposal, but it is not as simple as >> it is written above. We use existing insn as a testing place, where we >> stuff various combinations and call recog. After all processing, we >> have to restore insn to its original state and singal >> recog_for_combine caller that we recognized the combination, but with >> optional clobbers. So, calling sites only expect the >> confirmation/rejection of the proposed patterns, and in case of >> confirmation, recog_for_combine is allowed to decorate the pattern >> with optional clobbers. > > > If I understand you correctly, problems would arise if such a hook > produced a pattern that is *not* recognized. In such a case combine > performs a roll-back, and with a changed pattern some assumptions > will break? Yes, please see the code after validate_replacement: label in combine.c where it it assumed that recog_for_combine strips clobbers on the failure (or adds unknown number of clobbers on success). You will see that various more or less complex transformations happen after this label using SUBST and SUBST_MODE macros. On the failure, the combined pattern is changed/transformed in some way and recog_for_combine is called again. If recognition fails, another transformation takes place, etc... > Are there also problems if the hook transformed to a pattern that > is known to be recognized? Like "I know this pattern, just > canonicalize it to an expression that will match". In that case > no roll-back is needed. For the hook - yes, since the pattern will be changed in a way that is not expected by code that calls recog_for_combine. Your proposed transformation should happen after validate_replacement: label. However, this change is out of the scope of my proposed patch, this feature is not related to the ability to reject invalid insns. BTW: The calling code expects recog_for_combine to strip clobbers from the tested combined pattern on failure. This was not the case in my patch, so please expect v2 shortly. Uros.
Index: combine.c =================================================================== --- combine.c (revision 190500) +++ combine.c (working copy) @@ -10507,6 +10507,7 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn int i; rtx notes = 0; 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 @@ -10566,6 +10567,7 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn print_rtl_single (dump_file, pat); } } + PATTERN (insn) = old_pat; REG_NOTES (insn) = old_notes; @@ -10607,6 +10609,29 @@ 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); + 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 190500) +++ doc/tm.texi (working copy) @@ -10938,6 +10938,12 @@ 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 insn +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 190500) +++ doc/tm.texi.in (working copy) @@ -10796,6 +10796,12 @@ 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 + +Take an instruction in @var{insn} and return @code{true} if the insn +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: target.def =================================================================== --- target.def (revision 190500) +++ target.def (working copy) @@ -1984,7 +1984,15 @@ 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, + "", + 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\