Re: [PATCH] BZ60501: Add addptr optab
> I agree with Vlad that we're better off with Andreas' patch than without,
> since
> computing addresses is going to be 99% of what reload/LRA needs to do.
>
> I also agree with Eric that some better commentary would be nice.
Ok. I've applied the following patch.
Bye,
-Andreas-
2014-03-24 Andreas Krebbel
PR rtl-optimization/60501
* optabs.def (addptr3_optab): New optab.
* optabs.c (gen_addptr3_insn, have_addptr3_insn): New function.
* doc/md.texi ("addptrm3"): Document new RTL standard expander.
* expr.h (gen_addptr3_insn, have_addptr3_insn): Add prototypes.
* lra.c (emit_add3_insn): Use the addptr pattern if available.
* config/s390/s390.md ("addptrdi3", "addptrsi3"): New expanders.
diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index 76902b5..7d9d1ad 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -5034,6 +5034,57 @@
[(set_attr "op_type" ",RXE")
(set_attr "type" "fsimp")])
+;
+; Pointer add instruction patterns
+;
+
+; This will match "*la_64"
+(define_expand "addptrdi3"
+ [(set (match_operand:DI 0 "register_operand" "")
+(plus:DI (match_operand:DI 1 "register_operand" "")
+(match_operand:DI 2 "nonmemory_operand" "")))]
+ "TARGET_64BIT"
+{
+ HOST_WIDE_INT c = INTVAL (operands[2]);
+
+ if (GET_CODE (operands[2]) == CONST_INT)
+{
+ if (!CONST_OK_FOR_CONSTRAINT_P (c, 'K', "K")
+ && !CONST_OK_FOR_CONSTRAINT_P (c, 'O', "Os"))
+{
+ operands[2] = force_const_mem (DImode, operands[2]);
+ operands[2] = force_reg (DImode, operands[2]);
+}
+ else if (!DISP_IN_RANGE (INTVAL (operands[2])))
+operands[2] = force_reg (DImode, operands[2]);
+}
+})
+
+; For 31 bit we have to prevent the generated pattern from matching
+; normal ADDs since la only does a 31 bit add. This is supposed to
+; match "force_la_31".
+(define_expand "addptrsi3"
+ [(parallel
+[(set (match_operand:SI 0 "register_operand" "")
+ (plus:SI (match_operand:SI 1 "register_operand" "")
+ (match_operand:SI 2 "nonmemory_operand" "")))
+ (use (const_int 0))])]
+ "!TARGET_64BIT"
+{
+ HOST_WIDE_INT c = INTVAL (operands[2]);
+
+ if (GET_CODE (operands[2]) == CONST_INT)
+{
+ if (!CONST_OK_FOR_CONSTRAINT_P (c, 'K', "K")
+ && !CONST_OK_FOR_CONSTRAINT_P (c, 'O', "Os"))
+{
+ operands[2] = force_const_mem (SImode, operands[2]);
+ operands[2] = force_reg (SImode, operands[2]);
+}
+ else if (!DISP_IN_RANGE (INTVAL (operands[2])))
+operands[2] = force_reg (SImode, operands[2]);
+}
+})
;;
;;- Subtract instructions.
diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 746acc2..85fd4b9 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
operand 0. All operands
must have mode @var{m}. This can be used even on two-address machines, by
means of constraints requiring operands 1 and 0 to be the same location.
+@cindex @code{addptr@var{m}3} instruction pattern
+@item @samp{addptr@var{m}3}
+Like @code{add@var{m}3} but is guaranteed to only be used for address
+calculations. The expanded code is not allowed to clobber the
+condition code. It only needs to be defined if @code{add@var{m}3}
+sets the condition code. If adds used for address calculations and
+normal adds are not compatible it is required to expand a distinct
+pattern (e.g. using an unspec). The pattern is used by LRA to emit
+address calculations. @code{add@var{m}3} is used if
+@code{addptr@var{m}3} is not defined.
+
@cindex @code{ssadd@var{m}3} instruction pattern
@cindex @code{usadd@var{m}3} instruction pattern
@cindex @code{sub@var{m}3} instruction pattern
diff --git a/gcc/expr.h b/gcc/expr.h
index 5111f06..524da67 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -180,10 +180,12 @@ extern void emit_libcall_block (rtx, rtx, rtx, rtx);
Likewise for subtraction and for just copying. */
extern rtx gen_add2_insn (rtx, rtx);
extern rtx gen_add3_insn (rtx, rtx, rtx);
+extern rtx gen_addptr3_insn (rtx, rtx, rtx);
extern rtx gen_sub2_insn (rtx, rtx);
extern rtx gen_sub3_insn (rtx, rtx, rtx);
extern rtx gen_move_insn (rtx, rtx);
extern int have_add2_insn (rtx, rtx);
+extern int have_addptr3_insn (rtx, rtx, rtx);
extern int have_sub2_insn (rtx, rtx);
/* Emit a pair of rtl insns to compare two rtx's and to jump
diff --git a/gcc/lra.c b/gcc/lra.c
index 77074e2..c1b92d8 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -254,6 +254,19 @@ emit_add3_insn (rtx x, rtx y, rtx z)
rtx insn, last;
last = get_last_insn ();
+
+ if (have_addptr3_insn (x, y, z))
+{
+ insn = gen_addptr3_insn (x, y, z);
+
+ /* If the target provides an "addptr" pattern it hopefully does
+for a reason. So falling back to the normal add would be
+a bug. */
+ lra_assert (insn != NULL_RTX);
+ emi
Re: [PATCH] BZ60501: Add addptr optab
On 03/18/2014 04:59 AM, Jakub Jelinek wrote: > On Mon, Mar 17, 2014 at 03:24:14PM -0400, Vladimir Makarov wrote: >> It is complicated. There is no guarantee that it is used only for >> addresses. I need some time to think how to fix it. >> >> Meanwhile, you *should* commit the patch into the trunk because it >> solves the real problem. And I can work from this to make changes >> that the new pattern is only used for addresses. >> >> The patch is absolutely safe for all targets but s390. There is >> still a tiny possibility that it might result in some problems for >> s390 (now I see only one situation when a pseudo in a subreg >> changed by equiv plus expr needs a reload). In any case your patch >> solves real numerous failures and can be used as a base for further >> work. >> >> Thanks for working on this problem, Andreas. Sorry that I missed >> the PR60501. > > BTW, does LRA require that CC isn't clobbered when it uses > emit_add2_insn? I don't see how it can be guaranteed > (except perhaps on i?86/x86_64 and maybe a few other targets). At least in pre-LRA days one *had* to have such an addition, or alternately you had to leave compare+use as a single insn pattern until post-reload splitting. I assume that's really unchanged with LRA... > emit_add3_insn should be ok (even on s390*?) because recog_memoized > should (I think) never add clobbers (it calls recog with 0 > as last argument), but gen_add2_insn is a normall add3 insn that > on many targets clobbers CC. Sure, but s390 has no choice but to implement the clobber-less add with the LOAD ADDRESS instruction. Which is perfectly safe for 64-bit, but for 32-bit will in fact crop that 31st bit. Which is perfectly safe so long as we never want to do addition except on pointers. Which brings us back to Vlad's point about REG_EQUIV being a potential hazzard. I agree with Vlad that we're better off with Andreas' patch than without, since computing addresses is going to be 99% of what reload/LRA needs to do. I also agree with Eric that some better commentary would be nice. r~
Re: [PATCH] BZ60501: Add addptr optab
On Mon, Mar 17, 2014 at 03:24:14PM -0400, Vladimir Makarov wrote: > It is complicated. There is no guarantee that it is used only for > addresses. I need some time to think how to fix it. > > Meanwhile, you *should* commit the patch into the trunk because it > solves the real problem. And I can work from this to make changes > that the new pattern is only used for addresses. > > The patch is absolutely safe for all targets but s390. There is > still a tiny possibility that it might result in some problems for > s390 (now I see only one situation when a pseudo in a subreg > changed by equiv plus expr needs a reload). In any case your patch > solves real numerous failures and can be used as a base for further > work. > > Thanks for working on this problem, Andreas. Sorry that I missed > the PR60501. BTW, does LRA require that CC isn't clobbered when it uses emit_add2_insn? I don't see how it can be guaranteed (except perhaps on i?86/x86_64 and maybe a few other targets). emit_add3_insn should be ok (even on s390*?) because recog_memoized should (I think) never add clobbers (it calls recog with 0 as last argument), but gen_add2_insn is a normall add3 insn that on many targets clobbers CC. Jakub
Re: [PATCH] BZ60501: Add addptr optab
On 2014-03-13, 7:37 AM, Andreas Krebbel wrote:
On 13/03/14 12:25, Richard Biener wrote:
On Thu, Mar 13, 2014 at 12:16 PM, Eric Botcazou wrote:
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
operand 0. All operands must have mode @var{m}. This can be used even on
two-address machines, by means of constraints requiring operands 1 and 0 to
be the same location.
+@cindex @code{addptr@var{m}3} instruction pattern
+@item @samp{addptr@var{m}3}
+Like @code{addptr@var{m}3} but does never clobber the condition code.
+It only needs to be defined if @code{add@var{m}3} either sets the
+condition code or address calculations cannot be performed with the
+normal add instructions due to other reasons. If adds used for
+address calculations and normal adds are not compatible it is required
+to expand a distinct pattern (e.g. using an unspec). The pattern is
+used by LRA to emit address calculations. @code{add@var{m}3} is used
+if @code{addptr@var{m}3} is not defined.
I'm a bit skeptical of the "address calculations cannot be performed with the
normal add instructions due to other reasons" part". Surely they can be
performed on all architectures supported by GCC as of this writing, otherwise
how would the compiler even work? And if it's really like @code{add@var{m}3},
why restricting it to addresses, i.e. why calling it @code{addptr@var{m}3}?
Does that come from an implementation constraint on s390 that supports it only
for a subset of the cases supported by @code{add@var{m}3}?
Yeah, isn't it that you want a named pattern like add_nocc for an add
that doesn't clobber flags?
This would suggest that you can use the pattern also for performing a normal
add in case the
condition code is not needed afterwards but this isn't correct for s390 31 bit
where an address
calculation is actually something different. addptr is better I think because
it is a pattern which
is supposed to be implemented with a load address instruction and the
middle-end guarantees to use
it only on addresses. (I hope LRA is actually behaving that way). Perhaps
loadptr or la or
loadaddress would be a better name?
It is complicated. There is no guarantee that it is used only for
addresses. I need some time to think how to fix it.
Meanwhile, you *should* commit the patch into the trunk because it
solves the real problem. And I can work from this to make changes that
the new pattern is only used for addresses.
The patch is absolutely safe for all targets but s390. There is still a
tiny possibility that it might result in some problems for s390 (now I
see only one situation when a pseudo in a subreg changed by equiv plus
expr needs a reload). In any case your patch solves real numerous
failures and can be used as a base for further work.
Thanks for working on this problem, Andreas. Sorry that I missed the
PR60501.
Re: [PATCH] BZ60501: Add addptr optab
> > Then you should document that by stating that the pattern is guaranteed > > to be invoked only for addresses (and may not clobber the condition > > code). > Ok, will do. Thanks. > > Hoping isn't sufficient IMO here, you need to rename/rework > > emit_add3_insn and possibly stop the compiler if the value it is > > invoked on is not an address. > Agreed. Any idea how to check for this? The head comment of lra_emit_add appears to be saying that it is invoked only for addresses; it's clear when it is invoked from base_plus_disp_to_reg but it's less clear when it is invoked from lra_emit_move. So a possible approach could be to split lra_emit_move into 2 parts: lra_emit_move with an assertion that y isn't a PLUS and lra_emit_address_move with the call to lra_emit_add, and review all the callers. If that's too much work, just copy (part of) the head comment of lra_emit_add to that of emit_add3_insn and cross your fingers (yes, the latter action adds something over just hoping :-) -- Eric Botcazou
Re: [PATCH] BZ60501: Add addptr optab
On 14/03/14 11:02, Eric Botcazou wrote: >> This would suggest that you can use the pattern also for performing a normal >> add in case the condition code is not needed afterwards but this isn't >> correct for s390 31 bit where an address calculation is actually something >> different. > > Then you should document that by stating that the pattern is guaranteed to be > invoked only for addresses (and may not clobber the condition code). Ok, will do. >> addptr is better I think because it is a pattern which is >> supposed to be implemented with a load address instruction and the >> middle-end guarantees to use it only on addresses. (I hope LRA is actually >> behaving that way). > > Hoping isn't sufficient IMO here, you need to rename/rework emit_add3_insn > and > possibly stop the compiler if the value it is invoked on is not an address. Agreed. Any idea how to check for this? Bye, -Andreas-
Re: [PATCH] BZ60501: Add addptr optab
> This would suggest that you can use the pattern also for performing a normal > add in case the condition code is not needed afterwards but this isn't > correct for s390 31 bit where an address calculation is actually something > different. Then you should document that by stating that the pattern is guaranteed to be invoked only for addresses (and may not clobber the condition code). > addptr is better I think because it is a pattern which is > supposed to be implemented with a load address instruction and the > middle-end guarantees to use it only on addresses. (I hope LRA is actually > behaving that way). Hoping isn't sufficient IMO here, you need to rename/rework emit_add3_insn and possibly stop the compiler if the value it is invoked on is not an address. -- Eric Botcazou
Re: [PATCH] BZ60501: Add addptr optab
On 13/03/14 12:25, Richard Biener wrote:
> On Thu, Mar 13, 2014 at 12:16 PM, Eric Botcazou wrote:
>>> --- a/gcc/doc/md.texi
>>> +++ b/gcc/doc/md.texi
>>> @@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
>>> operand 0. All operands must have mode @var{m}. This can be used even on
>>> two-address machines, by means of constraints requiring operands 1 and 0 to
>>> be the same location.
>>>
>>> +@cindex @code{addptr@var{m}3} instruction pattern
>>> +@item @samp{addptr@var{m}3}
>>> +Like @code{addptr@var{m}3} but does never clobber the condition code.
>>> +It only needs to be defined if @code{add@var{m}3} either sets the
>>> +condition code or address calculations cannot be performed with the
>>> +normal add instructions due to other reasons. If adds used for
>>> +address calculations and normal adds are not compatible it is required
>>> +to expand a distinct pattern (e.g. using an unspec). The pattern is
>>> +used by LRA to emit address calculations. @code{add@var{m}3} is used
>>> +if @code{addptr@var{m}3} is not defined.
>>
>> I'm a bit skeptical of the "address calculations cannot be performed with the
>> normal add instructions due to other reasons" part". Surely they can be
>> performed on all architectures supported by GCC as of this writing, otherwise
>> how would the compiler even work? And if it's really like
>> @code{add@var{m}3},
>> why restricting it to addresses, i.e. why calling it @code{addptr@var{m}3}?
>> Does that come from an implementation constraint on s390 that supports it
>> only
>> for a subset of the cases supported by @code{add@var{m}3}?
>
> Yeah, isn't it that you want a named pattern like add_nocc for an add
> that doesn't clobber flags?
This would suggest that you can use the pattern also for performing a normal
add in case the
condition code is not needed afterwards but this isn't correct for s390 31 bit
where an address
calculation is actually something different. addptr is better I think because
it is a pattern which
is supposed to be implemented with a load address instruction and the
middle-end guarantees to use
it only on addresses. (I hope LRA is actually behaving that way). Perhaps
loadptr or la or
loadaddress would be a better name?
Bye,
-Andreas-
>
> Richard.
>
>>> diff --git a/gcc/lra.c b/gcc/lra.c
>>> index 77074e2..e5e81474 100644
>>> --- a/gcc/lra.c
>>> +++ b/gcc/lra.c
>>> @@ -254,6 +254,19 @@ emit_add3_insn (rtx x, rtx y, rtx z)
>>>rtx insn, last;
>>>
>>>last = get_last_insn ();
>>> +
>>> + if (have_addptr3_insn (x, y, z))
>>> +{
>>> + insn = gen_addptr3_insn (x, y, z);
>>> +
>>> + /* If the target provides an "addptr" pattern it hopefully does
>>> + for a reason. So falling back to the normal add would be
>>> + a bug. */
>>> + lra_assert (insn != NULL_RTX);
>>> + emit_insn (insn);
>>> + return insn;
>>> +}
>>> +
>>>insn = emit_insn (gen_rtx_SET (VOIDmode, x,
>>>gen_rtx_PLUS (GET_MODE (y), y, z)));
>>>if (recog_memoized (insn) < 0)
>>
>> Same ambiguity here, emit_add3_insn is not documented as being restricted to
>> addresses:
>>
>> /* Emit insn x = y + z. Return NULL if we failed to do it.
>>Otherwise, return the insn. We don't use gen_add3_insn as it might
>>clobber CC. */
>> static rtx
>> emit_add3_insn (rtx x, rtx y, rtx z)
>>
>> --
>> Eric Botcazou
>
Re: [PATCH] BZ60501: Add addptr optab
On Thu, Mar 13, 2014 at 12:16 PM, Eric Botcazou wrote:
>> --- a/gcc/doc/md.texi
>> +++ b/gcc/doc/md.texi
>> @@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
>> operand 0. All operands must have mode @var{m}. This can be used even on
>> two-address machines, by means of constraints requiring operands 1 and 0 to
>> be the same location.
>>
>> +@cindex @code{addptr@var{m}3} instruction pattern
>> +@item @samp{addptr@var{m}3}
>> +Like @code{addptr@var{m}3} but does never clobber the condition code.
>> +It only needs to be defined if @code{add@var{m}3} either sets the
>> +condition code or address calculations cannot be performed with the
>> +normal add instructions due to other reasons. If adds used for
>> +address calculations and normal adds are not compatible it is required
>> +to expand a distinct pattern (e.g. using an unspec). The pattern is
>> +used by LRA to emit address calculations. @code{add@var{m}3} is used
>> +if @code{addptr@var{m}3} is not defined.
>
> I'm a bit skeptical of the "address calculations cannot be performed with the
> normal add instructions due to other reasons" part". Surely they can be
> performed on all architectures supported by GCC as of this writing, otherwise
> how would the compiler even work? And if it's really like @code{add@var{m}3},
> why restricting it to addresses, i.e. why calling it @code{addptr@var{m}3}?
> Does that come from an implementation constraint on s390 that supports it only
> for a subset of the cases supported by @code{add@var{m}3}?
Yeah, isn't it that you want a named pattern like add_nocc for an add
that doesn't clobber flags?
Richard.
>> diff --git a/gcc/lra.c b/gcc/lra.c
>> index 77074e2..e5e81474 100644
>> --- a/gcc/lra.c
>> +++ b/gcc/lra.c
>> @@ -254,6 +254,19 @@ emit_add3_insn (rtx x, rtx y, rtx z)
>>rtx insn, last;
>>
>>last = get_last_insn ();
>> +
>> + if (have_addptr3_insn (x, y, z))
>> +{
>> + insn = gen_addptr3_insn (x, y, z);
>> +
>> + /* If the target provides an "addptr" pattern it hopefully does
>> + for a reason. So falling back to the normal add would be
>> + a bug. */
>> + lra_assert (insn != NULL_RTX);
>> + emit_insn (insn);
>> + return insn;
>> +}
>> +
>>insn = emit_insn (gen_rtx_SET (VOIDmode, x,
>>gen_rtx_PLUS (GET_MODE (y), y, z)));
>>if (recog_memoized (insn) < 0)
>
> Same ambiguity here, emit_add3_insn is not documented as being restricted to
> addresses:
>
> /* Emit insn x = y + z. Return NULL if we failed to do it.
>Otherwise, return the insn. We don't use gen_add3_insn as it might
>clobber CC. */
> static rtx
> emit_add3_insn (rtx x, rtx y, rtx z)
>
> --
> Eric Botcazou
Re: [PATCH] BZ60501: Add addptr optab
On 13/03/14 12:16, Eric Botcazou wrote:
>> --- a/gcc/doc/md.texi
>> +++ b/gcc/doc/md.texi
>> @@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
>> operand 0. All operands must have mode @var{m}. This can be used even on
>> two-address machines, by means of constraints requiring operands 1 and 0 to
>> be the same location.
>>
>> +@cindex @code{addptr@var{m}3} instruction pattern
>> +@item @samp{addptr@var{m}3}
>> +Like @code{addptr@var{m}3} but does never clobber the condition code.
>> +It only needs to be defined if @code{add@var{m}3} either sets the
>> +condition code or address calculations cannot be performed with the
>> +normal add instructions due to other reasons. If adds used for
>> +address calculations and normal adds are not compatible it is required
>> +to expand a distinct pattern (e.g. using an unspec). The pattern is
>> +used by LRA to emit address calculations. @code{add@var{m}3} is used
>> +if @code{addptr@var{m}3} is not defined.
>
> I'm a bit skeptical of the "address calculations cannot be performed with the
> normal add instructions due to other reasons" part". Surely they can be
> performed on all architectures supported by GCC as of this writing, otherwise
> how would the compiler even work?
For S/390 it really is only the condition code clobber why the normal add
cannot be used here.
Apart from that an add can do everything an load adress can do. I don't have
an example for other
reasons which might require this. So I can remove it.
> And if it's really like @code{add@var{m}3},
> why restricting it to addresses, i.e. why calling it @code{addptr@var{m}3}?
> Does that come from an implementation constraint on s390 that supports it
> only
> for a subset of the cases supported by @code{add@var{m}3}?
For S/390 31 bit an address calculation cannot be used as a normal add. An la
is actually only a 31
bit operation.
Bye,
-Andreas-
>
>> diff --git a/gcc/lra.c b/gcc/lra.c
>> index 77074e2..e5e81474 100644
>> --- a/gcc/lra.c
>> +++ b/gcc/lra.c
>> @@ -254,6 +254,19 @@ emit_add3_insn (rtx x, rtx y, rtx z)
>>rtx insn, last;
>>
>>last = get_last_insn ();
>> +
>> + if (have_addptr3_insn (x, y, z))
>> +{
>> + insn = gen_addptr3_insn (x, y, z);
>> +
>> + /* If the target provides an "addptr" pattern it hopefully does
>> + for a reason. So falling back to the normal add would be
>> + a bug. */
>> + lra_assert (insn != NULL_RTX);
>> + emit_insn (insn);
>> + return insn;
>> +}
>> +
>>insn = emit_insn (gen_rtx_SET (VOIDmode, x,
>> gen_rtx_PLUS (GET_MODE (y), y, z)));
>>if (recog_memoized (insn) < 0)
>
> Same ambiguity here, emit_add3_insn is not documented as being restricted to
> addresses:
>
> /* Emit insn x = y + z. Return NULL if we failed to do it.
>Otherwise, return the insn. We don't use gen_add3_insn as it might
>clobber CC. */
> static rtx
> emit_add3_insn (rtx x, rtx y, rtx z)
>
Re: [PATCH] BZ60501: Add addptr optab
> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
> operand 0. All operands must have mode @var{m}. This can be used even on
> two-address machines, by means of constraints requiring operands 1 and 0 to
> be the same location.
>
> +@cindex @code{addptr@var{m}3} instruction pattern
> +@item @samp{addptr@var{m}3}
> +Like @code{addptr@var{m}3} but does never clobber the condition code.
> +It only needs to be defined if @code{add@var{m}3} either sets the
> +condition code or address calculations cannot be performed with the
> +normal add instructions due to other reasons. If adds used for
> +address calculations and normal adds are not compatible it is required
> +to expand a distinct pattern (e.g. using an unspec). The pattern is
> +used by LRA to emit address calculations. @code{add@var{m}3} is used
> +if @code{addptr@var{m}3} is not defined.
I'm a bit skeptical of the "address calculations cannot be performed with the
normal add instructions due to other reasons" part". Surely they can be
performed on all architectures supported by GCC as of this writing, otherwise
how would the compiler even work? And if it's really like @code{add@var{m}3},
why restricting it to addresses, i.e. why calling it @code{addptr@var{m}3}?
Does that come from an implementation constraint on s390 that supports it only
for a subset of the cases supported by @code{add@var{m}3}?
> diff --git a/gcc/lra.c b/gcc/lra.c
> index 77074e2..e5e81474 100644
> --- a/gcc/lra.c
> +++ b/gcc/lra.c
> @@ -254,6 +254,19 @@ emit_add3_insn (rtx x, rtx y, rtx z)
>rtx insn, last;
>
>last = get_last_insn ();
> +
> + if (have_addptr3_insn (x, y, z))
> +{
> + insn = gen_addptr3_insn (x, y, z);
> +
> + /* If the target provides an "addptr" pattern it hopefully does
> + for a reason. So falling back to the normal add would be
> + a bug. */
> + lra_assert (insn != NULL_RTX);
> + emit_insn (insn);
> + return insn;
> +}
> +
>insn = emit_insn (gen_rtx_SET (VOIDmode, x,
>gen_rtx_PLUS (GET_MODE (y), y, z)));
>if (recog_memoized (insn) < 0)
Same ambiguity here, emit_add3_insn is not documented as being restricted to
addresses:
/* Emit insn x = y + z. Return NULL if we failed to do it.
Otherwise, return the insn. We don't use gen_add3_insn as it might
clobber CC. */
static rtx
emit_add3_insn (rtx x, rtx y, rtx z)
--
Eric Botcazou
Re: [PATCH] BZ60501: Add addptr optab
On Thu, Mar 13, 2014 at 10:24:13AM +0100, Andreas Krebbel wrote:
> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
> operand 0. All operands
> must have mode @var{m}. This can be used even on two-address machines, by
> means of constraints requiring operands 1 and 0 to be the same location.
>
> +@cindex @code{addptr@var{m}3} instruction pattern
> +@item @samp{addptr@var{m}3}
> +Like @code{addptr@var{m}3} but does never clobber the condition code.
Didn't you mean "Like @code{add@var{m}3}" here?
> +int
> +have_addptr3_insn (rtx x, rtx y, rtx z)
Missing function comment.
Otherwise looks good to me, but please give Vladimir, Jeff or Eric 24 hours to
comment on it.
Jakub
[PATCH] BZ60501: Add addptr optab
Hi,
fixes the LRA problems described in:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60501
and
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57604
Bootstrapped and regtested on s390, s390x, and x86_64.
Ok?
Bye,
-Andreas-
2014-03-13 Andreas Krebbel
PR rtl-optimization/60501
* optabs.def (addptr3_optab): New optab.
* optabs.c (gen_addptr3_insn, have_addptr3_insn): New function.
* doc/md.texi ("addptrm3"): Document new RTL standard expander.
* expr.h (gen_addptr3_insn, have_addptr3_insn): Add prototypes.
* lra.c (emit_add3_insn): Use the addptr pattern if available.
* config/s390/s390.md ("addptrdi3", "addptrsi3"): New expanders.
diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index 76902b5..7d9d1ad 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -5034,6 +5034,57 @@
[(set_attr "op_type" ",RXE")
(set_attr "type" "fsimp")])
+;
+; Pointer add instruction patterns
+;
+
+; This will match "*la_64"
+(define_expand "addptrdi3"
+ [(set (match_operand:DI 0 "register_operand" "")
+(plus:DI (match_operand:DI 1 "register_operand" "")
+(match_operand:DI 2 "nonmemory_operand" "")))]
+ "TARGET_64BIT"
+{
+ HOST_WIDE_INT c = INTVAL (operands[2]);
+
+ if (GET_CODE (operands[2]) == CONST_INT)
+{
+ if (!CONST_OK_FOR_CONSTRAINT_P (c, 'K', "K")
+ && !CONST_OK_FOR_CONSTRAINT_P (c, 'O', "Os"))
+{
+ operands[2] = force_const_mem (DImode, operands[2]);
+ operands[2] = force_reg (DImode, operands[2]);
+}
+ else if (!DISP_IN_RANGE (INTVAL (operands[2])))
+operands[2] = force_reg (DImode, operands[2]);
+}
+})
+
+; For 31 bit we have to prevent the generated pattern from matching
+; normal ADDs since la only does a 31 bit add. This is supposed to
+; match "force_la_31".
+(define_expand "addptrsi3"
+ [(parallel
+[(set (match_operand:SI 0 "register_operand" "")
+ (plus:SI (match_operand:SI 1 "register_operand" "")
+ (match_operand:SI 2 "nonmemory_operand" "")))
+ (use (const_int 0))])]
+ "!TARGET_64BIT"
+{
+ HOST_WIDE_INT c = INTVAL (operands[2]);
+
+ if (GET_CODE (operands[2]) == CONST_INT)
+{
+ if (!CONST_OK_FOR_CONSTRAINT_P (c, 'K', "K")
+ && !CONST_OK_FOR_CONSTRAINT_P (c, 'O', "Os"))
+{
+ operands[2] = force_const_mem (SImode, operands[2]);
+ operands[2] = force_reg (SImode, operands[2]);
+}
+ else if (!DISP_IN_RANGE (INTVAL (operands[2])))
+operands[2] = force_reg (SImode, operands[2]);
+}
+})
;;
;;- Subtract instructions.
diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 746acc2..972b717 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -4720,6 +4720,17 @@ Add operand 2 and operand 1, storing the result in
operand 0. All operands
must have mode @var{m}. This can be used even on two-address machines, by
means of constraints requiring operands 1 and 0 to be the same location.
+@cindex @code{addptr@var{m}3} instruction pattern
+@item @samp{addptr@var{m}3}
+Like @code{addptr@var{m}3} but does never clobber the condition code.
+It only needs to be defined if @code{add@var{m}3} either sets the
+condition code or address calculations cannot be performed with the
+normal add instructions due to other reasons. If adds used for
+address calculations and normal adds are not compatible it is required
+to expand a distinct pattern (e.g. using an unspec). The pattern is
+used by LRA to emit address calculations. @code{add@var{m}3} is used
+if @code{addptr@var{m}3} is not defined.
+
@cindex @code{ssadd@var{m}3} instruction pattern
@cindex @code{usadd@var{m}3} instruction pattern
@cindex @code{sub@var{m}3} instruction pattern
diff --git a/gcc/expr.h b/gcc/expr.h
index 5111f06..524da67 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -180,10 +180,12 @@ extern void emit_libcall_block (rtx, rtx, rtx, rtx);
Likewise for subtraction and for just copying. */
extern rtx gen_add2_insn (rtx, rtx);
extern rtx gen_add3_insn (rtx, rtx, rtx);
+extern rtx gen_addptr3_insn (rtx, rtx, rtx);
extern rtx gen_sub2_insn (rtx, rtx);
extern rtx gen_sub3_insn (rtx, rtx, rtx);
extern rtx gen_move_insn (rtx, rtx);
extern int have_add2_insn (rtx, rtx);
+extern int have_addptr3_insn (rtx, rtx, rtx);
extern int have_sub2_insn (rtx, rtx);
/* Emit a pair of rtl insns to compare two rtx's and to jump
diff --git a/gcc/lra.c b/gcc/lra.c
index 77074e2..e5e81474 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -254,6 +254,19 @@ emit_add3_insn (rtx x, rtx y, rtx z)
rtx insn, last;
last = get_last_insn ();
+
+ if (have_addptr3_insn (x, y, z))
+{
+ insn = gen_addptr3_insn (x, y, z);
+
+ /* If the target provides an "addptr" pattern it hopefully does
+for a reason. So falling back to the normal add would be
+a bug. */
+ lra_assert (insn != NULL_RTX);
+ emit_insn (insn);
+ return insn
