regrename: don't overflow insn_rr_info

regrename: don't overflow insn_rr_info

[Bernd Schmidt]

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

This one is a fix for something that could currently only affect c6x, 
but I have code that exposes it on i386.

When optionally gathering operand info in regrename, we can overflow the 
array in certain situations. This can occur when we have a situation 
where a value is constructed in multiple small registers and then 
accessed as a larger one (CDImode in the testcase I have). In that case 
we enter the "superset" path, which fails the involved chains, but the 
smaller pieces still all get seen by record_operand_use, and there may 
be more of them than MAX_REGS_PER_ADDRESS.

The following fixes it. Bootstrapped and tested  with -frename-registers 
enabled at -O1 on x86_64-linux. Ok?


Bernd

[-- Attachment #2: rr-fail-op.diff --]
[-- Type: text/x-patch, Size: 1353 bytes --]

	* regrename.c (record_operand_use): Keep track of failed operands
	and stop appending if we see any.
	* regrename.h (struct operand_rr_info): Add a failed field and shrink
	n_chains to short.

Index: gcc/regrename.c
===================================================================
--- gcc/regrename.c	(revision 229049)
+++ gcc/regrename.c	(working copy)
@@ -204,8 +204,13 @@ mark_conflict (struct du_head *chains, u
 static void
 record_operand_use (struct du_head *head, struct du_chain *this_du)
 {
-  if (cur_operand == NULL)
+  if (cur_operand == NULL || cur_operand->failed)
     return;
+  if (head->cannot_rename)
+    {
+      cur_operand->failed = true;
+      return;
+    }
   gcc_assert (cur_operand->n_chains < MAX_REGS_PER_ADDRESS);
   cur_operand->heads[cur_operand->n_chains] = head;
   cur_operand->chains[cur_operand->n_chains++] = this_du;
Index: gcc/regrename.h
===================================================================
--- gcc/regrename.h	(revision 229049)
+++ gcc/regrename.h	(working copy)
@@ -68,7 +71,8 @@ struct du_chain
 struct operand_rr_info
 {
   /* The number of chains recorded for this operand.  */
-  int n_chains;
+  short n_chains;
+  bool failed;
   /* Holds either the chain for the operand itself, or for the registers in
      a memory operand.  */
   struct du_chain *chains[MAX_REGS_PER_ADDRESS];

Re: regrename: don't overflow insn_rr_info

[Ramana Radhakrishnan]

On 06/11/15 11:08, Bernd Schmidt wrote:
> This one is a fix for something that could currently only affect c6x, but I have code that exposes it on i386.
> 
> When optionally gathering operand info in regrename, we can overflow the array in certain situations. This can occur when we have a situation where a value is constructed in multiple small registers and then accessed as a larger one (CDImode in the testcase I have). In that case we enter the "superset" path, which fails the involved chains, but the smaller pieces still all get seen by record_operand_use, and there may be more of them than MAX_REGS_PER_ADDRESS.
> 
> The following fixes it. Bootstrapped and tested  with -frename-registers enabled at -O1 on x86_64-linux. Ok?
> 
> 
> Bernd

This sounds like it will fix http://gcc.gnu.org/PR66785 ...

Ramana

Re: regrename: don't overflow insn_rr_info

[Bernd Schmidt]

On 11/06/2015 12:17 PM, Ramana Radhakrishnan wrote:
> On 06/11/15 11:08, Bernd Schmidt wrote:
>> This one is a fix for something that could currently only affect c6x, but I have code that exposes it on i386.
>>
>> When optionally gathering operand info in regrename, we can overflow the array in certain situations. This can occur when we have a situation where a value is constructed in multiple small registers and then accessed as a larger one (CDImode in the testcase I have). In that case we enter the "superset" path, which fails the involved chains, but the smaller pieces still all get seen by record_operand_use, and there may be more of them than MAX_REGS_PER_ADDRESS.
>>
>> The following fixes it. Bootstrapped and tested  with -frename-registers enabled at -O1 on x86_64-linux. Ok?
>>
>>
>> Bernd
>
> This sounds like it will fix http://gcc.gnu.org/PR66785 ...

Ah, I didn't realize something else was using this functionality:

gcc/config/aarch64/cortex-a57-fma-steering.c
1025:  regrename_init (true);

Yeah, the description of that bug makes it sound like the same issue.


Bernd

Re: regrename: don't overflow insn_rr_info

[Ramana Radhakrishnan]

On 06/11/15 11:31, Bernd Schmidt wrote:
> On 11/06/2015 12:17 PM, Ramana Radhakrishnan wrote:
>> On 06/11/15 11:08, Bernd Schmidt wrote:
>>> This one is a fix for something that could currently only affect c6x, but I have code that exposes it on i386.
>>>
>>> When optionally gathering operand info in regrename, we can overflow the array in certain situations. This can occur when we have a situation where a value is constructed in multiple small registers and then accessed as a larger one (CDImode in the testcase I have). In that case we enter the "superset" path, which fails the involved chains, but the smaller pieces still all get seen by record_operand_use, and there may be more of them than MAX_REGS_PER_ADDRESS.
>>>
>>> The following fixes it. Bootstrapped and tested  with -frename-registers enabled at -O1 on x86_64-linux. Ok?
>>>
>>>
>>> Bernd
>>
>> This sounds like it will fix http://gcc.gnu.org/PR66785 ...
> 
> Ah, I didn't realize something else was using this functionality:
> 
> gcc/config/aarch64/cortex-a57-fma-steering.c
> 1025:  regrename_init (true);
> 
> Yeah, the description of that bug makes it sound like the same issue.

Yeah looks like the ICE goes away with a quick spin - I've not done any deeper analysis but that looks like a fix.

I'll take the opportunity to point out gcc11{3-6} if you need an aarch64 machine on the compile farm if you wanted access to one.

regards
Ramana



> 
> 
> Bernd

Re: regrename: don't overflow insn_rr_info

[Jeff Law]

On 11/06/2015 04:08 AM, Bernd Schmidt wrote:
> This one is a fix for something that could currently only affect c6x,
> but I have code that exposes it on i386.
>
> When optionally gathering operand info in regrename, we can overflow the
> array in certain situations. This can occur when we have a situation
> where a value is constructed in multiple small registers and then
> accessed as a larger one (CDImode in the testcase I have). In that case
> we enter the "superset" path, which fails the involved chains, but the
> smaller pieces still all get seen by record_operand_use, and there may
> be more of them than MAX_REGS_PER_ADDRESS.
>
> The following fixes it. Bootstrapped and tested  with -frename-registers
> enabled at -O1 on x86_64-linux. Ok?
>
>
> Bernd
>
> rr-fail-op.diff
>
>
> 	* regrename.c (record_operand_use): Keep track of failed operands
> 	and stop appending if we see any.
> 	* regrename.h (struct operand_rr_info): Add a failed field and shrink
> 	n_chains to short.
OK.

jeff