Re: [PATCH][MSP430] Add support for post increment addressing
On 10/14/19 7:30 AM, Jozef Lawrynowicz wrote:
> On Sun, 13 Oct 2019 10:22:31 -0600
> Jeff Law wrote:
>
>> On 10/7/19 2:28 PM, Jozef Lawrynowicz wrote:
>>> MSP430 supports post increment addressing for the source operand only. The
>>> attached patch enables post increment addressing for MSP430 in GCC.
>>>
>>> Regtested on trunk for the small and large memory models.
>>>
>>> Ok for trunk?
>>>
>>>
>>> 0001-MSP430-Implement-post-increment-addressing.patch
>>>
>>> From d75e48ba434d7bab141c09d442793b2cb26afa69 Mon Sep 17 00:00:00 2001
>>> From: Jozef Lawrynowicz
>>> Date: Mon, 16 Sep 2019 16:34:51 +0100
>>> Subject: [PATCH] MSP430: Implement post increment addressing
>>>
>>> gcc/ChangeLog:
>>>
>>> 2019-10-07 Jozef Lawrynowicz
>>>
>>> * config/msp430/constraints.md: Allow post_inc operand for "Ya"
>>> constraint.
>>> * config/msp430/msp430.c (msp430_legitimate_address_p): Handle
>>> POST_INC.
>>> (msp430_subreg): Likewise.
>>> (msp430_split_addsi): Likewise.
>>> (msp430_print_operand_addr): Likewise.
>>> * config/msp430/msp430.h (HAVE_POST_INCREMENT): Define.
>>> (USE_STORE_POST_INCREMENT): Define.
>>> * config/msp430/msp430.md: Use the msp430_general_dst_operand or
>>> msp430_general_dst_nonv_operand predicates for the lvalues insns.
>>> * config/msp430/predicates.md (msp430_nonpostinc_operand): New.
>>> (msp430_general_dst_operand): New.
>>> (msp430_general_dst_nonv_operand): New.
>>> (msp430_nonsubreg_operand): Remove.
>>> (msp430_nonsubreg_dst_operand): New.
>>> (msp430_nonsubreg_or_imm_operand): Allow reg or mem operands in place
>>> of defunct msp430_nonsubreg_operand.
>>> (msp430_nonsubregnonpostinc_or_imm_operand): New.
>> So a high level question. The
>> USE_STORE_{PRE,POST}_{INCREMENT,DECREMENT} are primarily used within the
>> ivopts code to tune the addressing modes generated in there.
>>
>> To the best of my knowledge, they do not totally prevent using those
>> addressing modes elsewhere (auto-inc-dec.c) which instead rely strictly
>> on the HAVE_ macros and recognizing the result against the MD file.
>
> Additionally I don't think inc/dec RTXs will ever actually be generated unless
> they are handled in TARGET_LEGITIMATE_ADDRESS_P.
>
>>
>> So will these changes handle auto-increment addressing modes in
>> destination operands if they are generated by auto-inc-dec.c? Or have
>> you fixed all the predicates so that auto-inc-dec.c won't create them in
>> the first place?
>
> Yes, the new msp430_*_dst_operand predicates I've added will disallow a
> (mem (post_inc)).
>
>>
>>
>>
>> Based on a comment in msp430_split_addsi you have to handle stuff like
>>
>>> + (set (reg:SI)
>>> + (plus:SI (reg:SI)
>>> + (mem:SI (post_inc:PSI (reg:PSI).
>>
>> Do you need to check for and do anything special if the destination
>> operand is the same as the post-inc operand. Note RTX equality test is
>> probably not sufficient since you've got differing modes. Note this may
>> be affected by the prior question...
>
> This shouldn't ever happen since every lvalue operand which could be a mem
> has a
> dst_operand predicate. msp430_legitimate_address_p only allows post_inc in the
> format (post_inc (reg)) so we'll only ever get (mem (post_inc (reg))) RTXs.
>>
>> Are there any places where you could potentially have two source memory
>> operands? If so, do you need additional checks in those patterns?
>
> Are you referring to places where we have two source memory operands, neither
> of which are equal to the dest?
Yes.
>
> We don't have any insns that match this, since all msp430 instructions are
> either single or double operand.
Perfect.
>
> For expand patterns that could have two source memory operands,
> msp430_expand_helper will always move the operands into registers before
> calling a helper function.
>
> And the splitter for addsi is the only other place where we could have two
> source memory operands. This is where I put in two predicates which disallow
> post_inc, so there is only one possible position we can have it. IIRC, this
> case
> was actually exposed by running the testsuite :)
>
> Also, the assembler will emit the following error message if we ever do
> generate a post_inc for the dest operand:
>> t.s: Assembler messages:
>> t.s:2: Error: this addressing mode is not applicable for destination operand
>
> So I'm quite satisfied that can't/doesn't happen.
Perfect. THanks for walking me through things. OK for the trunk.
jeff
Re: [PATCH][MSP430] Add support for post increment addressing
On Sun, 13 Oct 2019 10:22:31 -0600
Jeff Law wrote:
> On 10/7/19 2:28 PM, Jozef Lawrynowicz wrote:
> > MSP430 supports post increment addressing for the source operand only. The
> > attached patch enables post increment addressing for MSP430 in GCC.
> >
> > Regtested on trunk for the small and large memory models.
> >
> > Ok for trunk?
> >
> >
> > 0001-MSP430-Implement-post-increment-addressing.patch
> >
> > From d75e48ba434d7bab141c09d442793b2cb26afa69 Mon Sep 17 00:00:00 2001
> > From: Jozef Lawrynowicz
> > Date: Mon, 16 Sep 2019 16:34:51 +0100
> > Subject: [PATCH] MSP430: Implement post increment addressing
> >
> > gcc/ChangeLog:
> >
> > 2019-10-07 Jozef Lawrynowicz
> >
> > * config/msp430/constraints.md: Allow post_inc operand for "Ya"
> > constraint.
> > * config/msp430/msp430.c (msp430_legitimate_address_p): Handle
> > POST_INC.
> > (msp430_subreg): Likewise.
> > (msp430_split_addsi): Likewise.
> > (msp430_print_operand_addr): Likewise.
> > * config/msp430/msp430.h (HAVE_POST_INCREMENT): Define.
> > (USE_STORE_POST_INCREMENT): Define.
> > * config/msp430/msp430.md: Use the msp430_general_dst_operand or
> > msp430_general_dst_nonv_operand predicates for the lvalues insns.
> > * config/msp430/predicates.md (msp430_nonpostinc_operand): New.
> > (msp430_general_dst_operand): New.
> > (msp430_general_dst_nonv_operand): New.
> > (msp430_nonsubreg_operand): Remove.
> > (msp430_nonsubreg_dst_operand): New.
> > (msp430_nonsubreg_or_imm_operand): Allow reg or mem operands in place
> > of defunct msp430_nonsubreg_operand.
> > (msp430_nonsubregnonpostinc_or_imm_operand): New.
> So a high level question. The
> USE_STORE_{PRE,POST}_{INCREMENT,DECREMENT} are primarily used within the
> ivopts code to tune the addressing modes generated in there.
>
> To the best of my knowledge, they do not totally prevent using those
> addressing modes elsewhere (auto-inc-dec.c) which instead rely strictly
> on the HAVE_ macros and recognizing the result against the MD file.
Additionally I don't think inc/dec RTXs will ever actually be generated unless
they are handled in TARGET_LEGITIMATE_ADDRESS_P.
>
> So will these changes handle auto-increment addressing modes in
> destination operands if they are generated by auto-inc-dec.c? Or have
> you fixed all the predicates so that auto-inc-dec.c won't create them in
> the first place?
Yes, the new msp430_*_dst_operand predicates I've added will disallow a
(mem (post_inc)).
>
>
>
> Based on a comment in msp430_split_addsi you have to handle stuff like
>
> > + (set (reg:SI)
> > + (plus:SI (reg:SI)
> > + (mem:SI (post_inc:PSI (reg:PSI).
>
> Do you need to check for and do anything special if the destination
> operand is the same as the post-inc operand. Note RTX equality test is
> probably not sufficient since you've got differing modes. Note this may
> be affected by the prior question...
This shouldn't ever happen since every lvalue operand which could be a mem has a
dst_operand predicate. msp430_legitimate_address_p only allows post_inc in the
format (post_inc (reg)) so we'll only ever get (mem (post_inc (reg))) RTXs.
>
> Are there any places where you could potentially have two source memory
> operands? If so, do you need additional checks in those patterns?
Are you referring to places where we have two source memory operands, neither
of which are equal to the dest?
We don't have any insns that match this, since all msp430 instructions are
either single or double operand.
For expand patterns that could have two source memory operands,
msp430_expand_helper will always move the operands into registers before
calling a helper function.
And the splitter for addsi is the only other place where we could have two
source memory operands. This is where I put in two predicates which disallow
post_inc, so there is only one possible position we can have it. IIRC, this case
was actually exposed by running the testsuite :)
Also, the assembler will emit the following error message if we ever do
generate a post_inc for the dest operand:
> t.s: Assembler messages:
> t.s:2: Error: this addressing mode is not applicable for destination operand
So I'm quite satisfied that can't/doesn't happen.
Thanks,
Jozef
>
>
> I've got no conceptual problem with the patch, I just want to make sure
> you've thought about those issues.
>
> jeff
>
>
>
Re: [PATCH][MSP430] Add support for post increment addressing
On 10/7/19 2:28 PM, Jozef Lawrynowicz wrote:
> MSP430 supports post increment addressing for the source operand only. The
> attached patch enables post increment addressing for MSP430 in GCC.
>
> Regtested on trunk for the small and large memory models.
>
> Ok for trunk?
>
>
> 0001-MSP430-Implement-post-increment-addressing.patch
>
> From d75e48ba434d7bab141c09d442793b2cb26afa69 Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz
> Date: Mon, 16 Sep 2019 16:34:51 +0100
> Subject: [PATCH] MSP430: Implement post increment addressing
>
> gcc/ChangeLog:
>
> 2019-10-07 Jozef Lawrynowicz
>
> * config/msp430/constraints.md: Allow post_inc operand for "Ya"
> constraint.
> * config/msp430/msp430.c (msp430_legitimate_address_p): Handle
> POST_INC.
> (msp430_subreg): Likewise.
> (msp430_split_addsi): Likewise.
> (msp430_print_operand_addr): Likewise.
> * config/msp430/msp430.h (HAVE_POST_INCREMENT): Define.
> (USE_STORE_POST_INCREMENT): Define.
> * config/msp430/msp430.md: Use the msp430_general_dst_operand or
> msp430_general_dst_nonv_operand predicates for the lvalues insns.
> * config/msp430/predicates.md (msp430_nonpostinc_operand): New.
> (msp430_general_dst_operand): New.
> (msp430_general_dst_nonv_operand): New.
> (msp430_nonsubreg_operand): Remove.
> (msp430_nonsubreg_dst_operand): New.
> (msp430_nonsubreg_or_imm_operand): Allow reg or mem operands in place
> of defunct msp430_nonsubreg_operand.
> (msp430_nonsubregnonpostinc_or_imm_operand): New.
So a high level question. The
USE_STORE_{PRE,POST}_{INCREMENT,DECREMENT} are primarily used within the
ivopts code to tune the addressing modes generated in there.
To the best of my knowledge, they do not totally prevent using those
addressing modes elsewhere (auto-inc-dec.c) which instead rely strictly
on the HAVE_ macros and recognizing the result against the MD file.
So will these changes handle auto-increment addressing modes in
destination operands if they are generated by auto-inc-dec.c? Or have
you fixed all the predicates so that auto-inc-dec.c won't create them in
the first place?
Based on a comment in msp430_split_addsi you have to handle stuff like
> + (set (reg:SI)
> + (plus:SI (reg:SI)
> +(mem:SI (post_inc:PSI (reg:PSI).
Do you need to check for and do anything special if the destination
operand is the same as the post-inc operand. Note RTX equality test is
probably not sufficient since you've got differing modes. Note this may
be affected by the prior question...
Are there any places where you could potentially have two source memory
operands? If so, do you need additional checks in those patterns?
I've got no conceptual problem with the patch, I just want to make sure
you've thought about those issues.
jeff
[PATCH][MSP430] Add support for post increment addressing
MSP430 supports post increment addressing for the source operand only. The
attached patch enables post increment addressing for MSP430 in GCC.
Regtested on trunk for the small and large memory models.
Ok for trunk?
>From d75e48ba434d7bab141c09d442793b2cb26afa69 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz
Date: Mon, 16 Sep 2019 16:34:51 +0100
Subject: [PATCH] MSP430: Implement post increment addressing
gcc/ChangeLog:
2019-10-07 Jozef Lawrynowicz
* config/msp430/constraints.md: Allow post_inc operand for "Ya"
constraint.
* config/msp430/msp430.c (msp430_legitimate_address_p): Handle
POST_INC.
(msp430_subreg): Likewise.
(msp430_split_addsi): Likewise.
(msp430_print_operand_addr): Likewise.
* config/msp430/msp430.h (HAVE_POST_INCREMENT): Define.
(USE_STORE_POST_INCREMENT): Define.
* config/msp430/msp430.md: Use the msp430_general_dst_operand or
msp430_general_dst_nonv_operand predicates for the lvalues insns.
* config/msp430/predicates.md (msp430_nonpostinc_operand): New.
(msp430_general_dst_operand): New.
(msp430_general_dst_nonv_operand): New.
(msp430_nonsubreg_operand): Remove.
(msp430_nonsubreg_dst_operand): New.
(msp430_nonsubreg_or_imm_operand): Allow reg or mem operands in place
of defunct msp430_nonsubreg_operand.
(msp430_nonsubregnonpostinc_or_imm_operand): New.
---
gcc/config/msp430/constraints.md | 1 +
gcc/config/msp430/msp430.c | 32 +-
gcc/config/msp430/msp430.h | 12 ++
gcc/config/msp430/msp430.md | 191 ---
gcc/config/msp430/predicates.md | 46 +++-
5 files changed, 183 insertions(+), 99 deletions(-)
diff --git a/gcc/config/msp430/constraints.md b/gcc/config/msp430/constraints.md
index 4422b2b6454..d01bcf9a242 100644
--- a/gcc/config/msp430/constraints.md
+++ b/gcc/config/msp430/constraints.md
@@ -60,6 +60,7 @@
(match_code "reg" "00")
(match_test ("CONST_INT_P (XEXP (XEXP (op, 0), 1))")))
(match_test "CONSTANT_P (XEXP (op, 0))")
+ (match_code "post_inc" "0")
)))
(define_constraint "Yl"
diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index ae763faada3..31029395c3d 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -942,12 +942,17 @@ msp430_legitimate_address_p (machine_mode mode ATTRIBUTE_UNUSED,
return false;
case PLUS:
+case POST_INC:
if (REG_P (XEXP (x, 0)))
{
if (GET_MODE (x) != GET_MODE (XEXP (x, 0)))
return false;
if (!reg_ok_for_addr (XEXP (x, 0), strict))
return false;
+ if (GET_CODE (x) == POST_INC)
+ /* At this point, if the original rtx was a post_inc, we don't have
+ anything further to check. */
+ return true;
switch (GET_CODE (XEXP (x, 1)))
{
case CONST:
@@ -2810,6 +2815,7 @@ rtx
msp430_subreg (machine_mode mode, rtx r, machine_mode omode, int byte)
{
rtx rv;
+ gcc_assert (mode == HImode);
if (GET_CODE (r) == SUBREG
&& SUBREG_BYTE (r) == 0)
@@ -2826,7 +2832,15 @@ msp430_subreg (machine_mode mode, rtx r, machine_mode omode, int byte)
rv = simplify_gen_subreg (mode, ireg, imode, byte);
}
else if (GET_CODE (r) == MEM)
-rv = adjust_address (r, mode, byte);
+{
+ /* When byte == 2, we can be certain that we were already called with an
+ identical rtx with byte == 0. So we don't need to do anything to
+ get a 2 byte offset of a (mem (post_inc)) rtx, since the address has
+ already been offset by the post_inc itself. */
+ if (GET_CODE (XEXP (r, 0)) == POST_INC && byte == 2)
+ byte = 0;
+ rv = adjust_address (r, mode, byte);
+}
else if (GET_CODE (r) == SYMBOL_REF
&& (byte == 0 || byte == 2)
&& mode == HImode)
@@ -2861,6 +2875,18 @@ msp430_split_addsi (rtx *operands)
if (GET_CODE (operands[5]) == CONST_INT)
operands[9] = GEN_INT (INTVAL (operands[5]) & 0x);
+ /* Handle post_inc, for example:
+ (set (reg:SI)
+ (plus:SI (reg:SI)
+ (mem:SI (post_inc:PSI (reg:PSI). */
+ else if (MEM_P (operands[5]) && GET_CODE (XEXP (operands[5], 0)) == POST_INC)
+{
+ /* Strip out the post_inc from (mem (post_inc (reg))). */
+ operands[9] = XEXP (XEXP (operands[5], 0), 0);
+ operands[9] = gen_rtx_MEM (HImode, operands[9]);
+ /* Then zero extend as normal. */
+ operands[9] = gen_rtx_ZERO_EXTEND (SImode, operands[9]);
+}
else
operands[9] = gen_rtx_ZERO_EXTEND (SImode, operands[5]);
return 0;
@@ -3205,6 +3231,10 @@ msp430_print_operand_addr (FILE * file, machine_mode /*mode*/, rtx addr)
fprintf (file, "@");
break;
+case POST_INC:
+ fprintf (file, "@%s+", reg_names[REGNO (XEXP (addr, 0))]);
+ return;
+
case CONST:
case CONST_INT:
case SYMBOL_REF:
diff --git a/gcc/config/msp430/msp430.h b/gcc/config/msp430/msp430.h
index f885de2bb2f..73afe2e2d16 100644
--- a/gcc/config/msp430/msp430.h
+++ b/gcc/config/msp430/msp430.h
@@ -478,6 +478,18 @@ typedef struct
#define
