fix regrename pass to ensure renamings produce valid insns

Hi all:

This patch seem will broken when disable assert checking for c6x....


On Mon, Jun 29, 2015 at 5:08 AM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> On 06/24/2015 09:46 PM, Jeff Law wrote:
>>
>> On 06/23/2015 07:00 PM, Sandra Loosemore wrote:
>>>
>>> On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>>>>>
>>>>> The attached patch teaches regrename to validate insns affected by each
>>>>> register renaming before making the change.  I can see at least two
>>>>> other ways to handle this -- earlier, by rejecting renamings that
>>>>> result
>>>>> in invalid instructions when it's searching for the best renaming; or
>>>>> later, by validating the entire set of renamings as a group instead of
>>>>> incrementally for each one -- but doing it all in regname_do_replace
>>>>> seems least disruptive and risky in terms of the existing code.
>>>>
>>>>
>>>> OK, but the patch looks incomplete, rename_chains should be adjusted
>>>> as well,
>>>> i.e. regrename_do_replace should now return a boolean.
>>>
>>>
>>> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
>>> built for aarch64-linux-gnu and ran the gcc testsuite.
>>>
>>> The c6x back end also calls regrename_do_replace.  I am not set up to
>>> build or test on that target, and Bernd told me off-list that it would
>>> never fail on that target anyway so I have left that code alone.
>>>
>>> -Sandra
>>>
>>> regrename-2.log
>>>
>>>
>>> 2015-06-23  Chung-Lin Tang<cltang@codesourcery.com>
>>>         Sandra Loosemore<sandra@codesourcery.com>
>>>
>>>     gcc/
>>>     * regrename.h (regrename_do_replace): Change to return bool.
>>>     * regrename.c (rename_chains): Check return value of
>>>     regname_do_replace.
>>>     (regrename_do_replace): Re-validate the modified insns and
>>>     return bool status.
>>>     * config/aarch64/cortex-a57-fma-steering.c (rename_single_chain):
>>>     Update to match rename_chains changes.
>>
>> As Eric mentioned, please put an assert to verify that the call from the
>> c6x backend never fails.
>>
>> The regrename and ARM bits are fine.
>>
>> Do you have a testcase that you can add to the suite?  If so it'd be
>> appreciated if you could include that too.
>>
>> Approved with the c6x assert if a testcase isn't available or
>> exceedingly difficult to produce.
>
>
> Thanks.  I've committed the attached version.
>
> Re the testcase, this fixed 16 FAILs on existing tests in the gcc testsuite
> with the forthcoming nios2 load/store multiple instruction support, all
> assembler errors due to the bad instructions being generated.  There's
> nothing I can do on nios2 for a testcase until I get those patches committed
> (I'm still trying to re-test and tidy them up for submission), plus I think
> the failures are rather fragile -- depending on the register allocator
> choosing an initial register numbering that allows peephole optimizers to
> trigger, etc.  But, I will revisit this later and see what I can do.
>
> -Sandra
>

On 06/29/2015 09:07 PM, Kito Cheng wrote:
> Hi all:
>
> This patch seem will broken when disable assert checking for c6x....
>
> Index: gcc/config/c6x/c6x.c
> ===================================================================
> --- gcc/config/c6x/c6x.c (revision 225104)
> +++ gcc/config/c6x/c6x.c (working copy)
> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
>     best_reg =
>       find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
>
> -  regrename_do_replace (this_head, best_reg);
> +  gcc_assert (regrename_do_replace (this_head, best_reg));
>
>     count_unit_reqs (new_reqs, head, PREV_INSN (tail));
>     merge_unit_reqs (new_reqs);
> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
>          unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
>       }
>     if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
> -    regrename_do_replace (this_head, old_reg);
> +    gcc_assert (regrename_do_replace (this_head, old_reg));
>     else
>       memcpy (reqs, new_reqs, sizeof (unit_req_table));
>

I'm sorry; do you have a suggestion for a fix?  I thought this was the 
change I was asked to make, and as I noted previously, I'm not set up to 
test (or even build) for this target.

-Sandra the obviously confused :-(

On 2015/6/30 12:22 PM, Sandra Loosemore wrote:
> On 06/29/2015 09:07 PM, Kito Cheng wrote:
>> Hi all:
>>
>> This patch seem will broken when disable assert checking for c6x....
>>
>> Index: gcc/config/c6x/c6x.c
>> ===================================================================
>> --- gcc/config/c6x/c6x.c (revision 225104)
>> +++ gcc/config/c6x/c6x.c (working copy)
>> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
>>     best_reg =
>>       find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
>>
>> -  regrename_do_replace (this_head, best_reg);
>> +  gcc_assert (regrename_do_replace (this_head, best_reg));
>>
>>     count_unit_reqs (new_reqs, head, PREV_INSN (tail));
>>     merge_unit_reqs (new_reqs);
>> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
>>          unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
>>       }
>>     if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
>> -    regrename_do_replace (this_head, old_reg);
>> +    gcc_assert (regrename_do_replace (this_head, old_reg));
>>     else
>>       memcpy (reqs, new_reqs, sizeof (unit_req_table));
>>
> 
> I'm sorry; do you have a suggestion for a fix?  I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target.
> 
> -Sandra the obviously confused :-(

You probably have to separate out the regrename_do_replace() bool result into a variable,
placing the whole call into the gcc_assert() might make it disappear when assertions are turned off.

Chung-Lin

On 2015/6/30 下午 01:13, Chung-Lin Tang wrote:
> On 2015/6/30 12:22 PM, Sandra Loosemore wrote:
>> On 06/29/2015 09:07 PM, Kito Cheng wrote:
>>> Hi all:
>>>
>>> This patch seem will broken when disable assert checking for c6x....
>>>
>>> Index: gcc/config/c6x/c6x.c
>>> ===================================================================
>>> --- gcc/config/c6x/c6x.c (revision 225104)
>>> +++ gcc/config/c6x/c6x.c (working copy)
>>> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
>>>     best_reg =
>>>       find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
>>>
>>> -  regrename_do_replace (this_head, best_reg);
>>> +  gcc_assert (regrename_do_replace (this_head, best_reg));
>>>
>>>     count_unit_reqs (new_reqs, head, PREV_INSN (tail));
>>>     merge_unit_reqs (new_reqs);
>>> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
>>>          unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
>>>       }
>>>     if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
>>> -    regrename_do_replace (this_head, old_reg);
>>> +    gcc_assert (regrename_do_replace (this_head, old_reg));
>>>     else
>>>       memcpy (reqs, new_reqs, sizeof (unit_req_table));
>>>
>>
>> I'm sorry; do you have a suggestion for a fix?  I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target.
>>
>> -Sandra the obviously confused :-(
> 
> You probably have to separate out the regrename_do_replace() bool result into a variable,
> placing the whole call into the gcc_assert() might make it disappear when assertions are turned off.

I notice the way gcc_assert() is defined in system.h now, the test won't disappear even when runtime checks
are disabled, though you might still adjust it to avoid any programmer confusion.

Chung-Lin

> I notice the way gcc_assert() is defined in system.h now, the test won't
> disappear even when runtime checks are disabled, though you might still
> adjust it to avoid any programmer confusion.

It will disappear at run time, see the definition:

/* Include EXPR, so that unused variable warnings do not occur.  */
#define gcc_assert(EXPR) ((void)(0 && (EXPR)))

so you really need to use a separate variable.

On 2015/6/30 05:06 PM, Eric Botcazou wrote:
>> I notice the way gcc_assert() is defined in system.h now, the test won't
>> disappear even when runtime checks are disabled, though you might still
>> adjust it to avoid any programmer confusion.
> 
> It will disappear at run time, see the definition:
> 
> /* Include EXPR, so that unused variable warnings do not occur.  */
> #define gcc_assert(EXPR) ((void)(0 && (EXPR)))
> 
> so you really need to use a separate variable.
> 

I was referring to this one:
#if ENABLE_ASSERT_CHECKING
...	
#elif (GCC_VERSION >= 4005)
#define gcc_assert(EXPR)                                                \
  ((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0))
#else
...

But yeah, I guess older GCCs could be used to build a toolchain,
so a separate variable should be used.

Chung-Lin