[RFA][PATCH] Fix mips16 codegen issue in a better way

[RFA][PATCH] Fix mips16 codegen issue in a better way

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]


[RFA is for the regcprop bits, reverting the mips.md hack seems like a 
no-brainer with the regcprop change. ]


Per the recent discussion between Richard S. and myself, this is a 
better fix for the mips16 codegen issue where it's creating invalid lwu 
insns.

As Richard S. pointed out there's been a long standing problem where 
regcprop would create a reference to the stack pointer that was unique 
from stack_pointer_rtx.  That's the root cause of the codegen issue.

We can't re-use stack_pointer_rtx in the code in question because we're 
going to modify the underlying RTX in fun and interesting ways.  Ports 
(such as the mips) assume references to the stack pointer are unique 
(ie, they can identify a stack pointer reference by stack_pointer_rtx 
rather than checking register #s).

So this patch just rejects propagation of the stack pointer.  It's 
conservative in that it doesn't reject other special registers.

An alternate approach would be to declare that ports can not depend on 
looking at stack_pointer_rtx to find all stack references that instead 
they have to look at the underlying regno.  The amount of auditing here 
would be significant.

I've bootstrapped and regression tested this on x86_64-linux-gnu.  It's 
also built libgcc/glibc/newlib on about 100 different targets.

OK for the trunk?

jeff

[-- Attachment #2: Q --]
[-- Type: text/plain, Size: 1995 bytes --]

diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
2017-04-17  Jeff Law  <law@redhat.com>

	* regcprop.c (maybe_mode_change): Avoid creating copies of the
	stack pointer.

	Revert:
	2017-04-13  Jeff Law  <law@redhat.com>
	* config/mips.mips.md (zero_extendsidi2): Do not allow SP to appear
	in operands[1] if it is a MEM and TARGET_MIPS16 is active.

index dd5e1e7..7acf00d 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -3493,10 +3493,7 @@
 (define_insn_and_split "*zero_extendsidi2"
   [(set (match_operand:DI 0 "register_operand" "=d,d")
         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "d,W")))]
-  "TARGET_64BIT && !ISA_HAS_EXT_INS
-   && !(TARGET_MIPS16
-        && MEM_P (operands[1])
-        && reg_mentioned_p (stack_pointer_rtx, operands[1]))"
+  "TARGET_64BIT && !ISA_HAS_EXT_INS"
   "@
    #
    lwu\t%0,%1"
@@ -3512,10 +3509,7 @@
 (define_insn "*zero_extendsidi2_dext"
   [(set (match_operand:DI 0 "register_operand" "=d,d")
         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "d,W")))]
-  "TARGET_64BIT && ISA_HAS_EXT_INS
-   && !(TARGET_MIPS16
-        && MEM_P (operands[1])
-        && reg_mentioned_p (stack_pointer_rtx, operands[1]))"
+  "TARGET_64BIT && ISA_HAS_EXT_INS"
   "@
    dext\t%0,%1,0,32
    lwu\t%0,%1"
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index ddc6252..367d85a 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -396,6 +396,13 @@ maybe_mode_change (machine_mode orig_mode, machine_mode copy_mode,
       && GET_MODE_SIZE (copy_mode) < GET_MODE_SIZE (new_mode))
     return NULL_RTX;
 
+  /* Avoid creating multiple copies of the stack pointer.  Some ports
+     assume there is one and only one stack pointer.
+
+     It's unclear if we need to do the same for other special registers.  */
+  if (regno == STACK_POINTER_REGNUM)
+    return NULL_RTX;
+
   if (orig_mode == new_mode)
     return gen_raw_REG (new_mode, regno);
   else if (mode_change_ok (orig_mode, new_mode, regno))

Re: [RFA][PATCH] Fix mips16 codegen issue in a better way

[Richard Sandiford]

Jeff Law <law@redhat.com> writes:
> [RFA is for the regcprop bits, reverting the mips.md hack seems like a 
> no-brainer with the regcprop change. ]
>
>
> Per the recent discussion between Richard S. and myself, this is a 
> better fix for the mips16 codegen issue where it's creating invalid lwu 
> insns.
>
> As Richard S. pointed out there's been a long standing problem where 
> regcprop would create a reference to the stack pointer that was unique 
> from stack_pointer_rtx.  That's the root cause of the codegen issue.
>
> We can't re-use stack_pointer_rtx in the code in question because we're 
> going to modify the underlying RTX in fun and interesting ways.  Ports 
> (such as the mips) assume references to the stack pointer are unique 
> (ie, they can identify a stack pointer reference by stack_pointer_rtx 
> rather than checking register #s).
>
> So this patch just rejects propagation of the stack pointer.  It's 
> conservative in that it doesn't reject other special registers.
>
> An alternate approach would be to declare that ports can not depend on 
> looking at stack_pointer_rtx to find all stack references that instead 
> they have to look at the underlying regno.  The amount of auditing here 
> would be significant.
>
> I've bootstrapped and regression tested this on x86_64-linux-gnu.  It's 
> also built libgcc/glibc/newlib on about 100 different targets.

Thanks for doing this, looks good to me FWIW.  I was wondering
whether we should use gen_rtx_REG instead of gen_raw_rtx_REG
in maybe_mode_change and then disable the assignment to things
like ORIGINAL_REGNO if the returned rtx is one of the global rtxes.
But I'm not sure how safe that would be.  We don't know whether the
original reference to the register number was the global rtx,
which might matter for things like hard_frame_pointer_rtx.

I think regcprop could be rejigged to handle global rtxes
"correctly", but that's obviously too invasive at this stage.

Richard

Re: [RFA][PATCH] Fix mips16 codegen issue in a better way

[Jeff Law]

On 04/18/2017 05:16 AM, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
>> [RFA is for the regcprop bits, reverting the mips.md hack seems like a
>> no-brainer with the regcprop change. ]
>>
>>
>> Per the recent discussion between Richard S. and myself, this is a
>> better fix for the mips16 codegen issue where it's creating invalid lwu
>> insns.
>>
>> As Richard S. pointed out there's been a long standing problem where
>> regcprop would create a reference to the stack pointer that was unique
>> from stack_pointer_rtx.  That's the root cause of the codegen issue.
>>
>> We can't re-use stack_pointer_rtx in the code in question because we're
>> going to modify the underlying RTX in fun and interesting ways.  Ports
>> (such as the mips) assume references to the stack pointer are unique
>> (ie, they can identify a stack pointer reference by stack_pointer_rtx
>> rather than checking register #s).
>>
>> So this patch just rejects propagation of the stack pointer.  It's
>> conservative in that it doesn't reject other special registers.
>>
>> An alternate approach would be to declare that ports can not depend on
>> looking at stack_pointer_rtx to find all stack references that instead
>> they have to look at the underlying regno.  The amount of auditing here
>> would be significant.
>>
>> I've bootstrapped and regression tested this on x86_64-linux-gnu.  It's
>> also built libgcc/glibc/newlib on about 100 different targets.
> 
> Thanks for doing this, looks good to me FWIW.  I was wondering
> whether we should use gen_rtx_REG instead of gen_raw_rtx_REG
> in maybe_mode_change and then disable the assignment to things
> like ORIGINAL_REGNO if the returned rtx is one of the global rtxes.
> But I'm not sure how safe that would be.  We don't know whether the
> original reference to the register number was the global rtx,
> which might matter for things like hard_frame_pointer_rtx.
I'd also be concerned that not setting something like REG_POINTER or 
REG_ATTRS could cause problems.

While I can probably get to "safe" for current intersection ports, 
special registers and how they interact with REG_POINTER, it feels 
rather fragile.

Jeff