Patchwork [middle-end] : Introduce TARGET_REJECT_COMBINED_INSN target hook

login
register
mail settings
Submitter Uros Bizjak
Date Aug. 23, 2012, 4:59 p.m.
Message ID <CAFULd4Y2NZXtpiyqLFWJc1r-H7qmrN7Gtb6q99V0P_FNXjyd5w@mail.gmail.com>
Download mbox | patch
Permalink /patch/179686/
State New
Headers show

Comments

Uros Bizjak - Aug. 23, 2012, 4:59 p.m.
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.

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.
Georg-Johann Lay - Aug. 23, 2012, 5:52 p.m.
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.
Andrew Pinski - Aug. 23, 2012, 5:59 p.m.
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.
>
Uros Bizjak - Aug. 23, 2012, 6:08 p.m.
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.
Uros Bizjak - Aug. 23, 2012, 6:23 p.m.
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.
Oleg Endo - Aug. 23, 2012, 6:27 p.m.
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
Joseph S. Myers - Aug. 23, 2012, 6:59 p.m.
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.)
Georg-Johann Lay - Aug. 23, 2012, 8:46 p.m.
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
Oleg Endo - Aug. 23, 2012, 10:13 p.m.
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
Uros Bizjak - Aug. 24, 2012, 6:17 a.m.
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.
Uros Bizjak - Aug. 24, 2012, 6:24 a.m.
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.
Georg-Johann Lay - Aug. 25, 2012, 9:55 a.m.
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
Uros Bizjak - Aug. 25, 2012, 1:37 p.m.
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.

Patch

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\