[PATCH][ARM][PR target/84826] Fix ICE in extract_insn, at recog.c:2304 on arm-linux-gnueabi
Sudakshina Das
sudi.das@arm.com
Fri Mar 23 14:17:00 GMT 2018
On 23/03/18 13:50, Kyrill Tkachov wrote:
>
> On 23/03/18 13:31, Sudakshina Das wrote:
>> On 23/03/18 09:12, Kyrill Tkachov wrote:
>>>
>>> On 23/03/18 08:47, Christophe Lyon wrote:
>>>> Hi Sudi,
>>>>
>>>>
>>>> On 22 March 2018 at 18:26, Sudakshina Das <sudi.das@arm.com> wrote:
>>>>> Hi
>>>>>
>>>>>
>>>>> On 22/03/18 16:52, Kyrill Tkachov wrote:
>>>>>>
>>>>>> On 22/03/18 16:20, Sudakshina Das wrote:
>>>>>>> Hi Kyrill
>>>>>>>
>>>>>>> On 22/03/18 16:08, Kyrill Tkachov wrote:
>>>>>>>> Hi Sudi,
>>>>>>>>
>>>>>>>> On 21/03/18 17:44, Sudakshina Das wrote:
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> The ICE in the bug report was happening because the macro
>>>>>>>>> USE_RETURN_INSN (FALSE) was returning different values at
>>>>>>>>> different
>>>>>>>>> points in the compilation. This was internally occurring
>>>>>>>>> because the
>>>>>>>>> function arm_compute_static_chain_stack_bytes () which was
>>>>>>>>> dependent on
>>>>>>>>> arm_r3_live_at_start_p () was giving a different value after the
>>>>>>>>> cond_exec instructions were created in ce3 causing the liveness
>>>>>>>>> of r3
>>>>>>>>> to escape up to the start block.
>>>>>>>>>
>>>>>>>>> The function arm_compute_static_chain_stack_bytes () should really
>>>>>>>>> only compute the value once during epilogue/prologue stage.
>>>>>>>>> This pass
>>>>>>>>> introduces a new member 'static_chain_stack_bytes' to the target
>>>>>>>>> definition of the struct machine_function which gets calculated in
>>>>>>>>> expand_prologue and is the value that is returned by
>>>>>>>>> arm_compute_static_chain_stack_bytes () beyond that.
>>>>>>>>>
>>>>>>>>> Testing done: Bootstrapped and regtested on
>>>>>>>>> arm-none-linux-gnueabihf
>>>>>>>>> and added the reported test to the testsuite.
>>>>>>>>>
>>>>>>>>> Is this ok for trunk?
>>>>>>>>>
>>>>>>>> Thanks for working on this.
>>>>>>>> I agree with the approach, I have a couple of comments inline.
>>>>>>>>
>>>>>>>>> Sudi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ChangeLog entries:
>>>>>>>>>
>>>>>>>>> *** gcc/ChangeLog ***
>>>>>>>>>
>>>>>>>>> 2018-03-21 Sudakshina Das <sudi.das@arm.com>
>>>>>>>>>
>>>>>>>>> Â Â Â Â Â Â Â Â PR target/84826
>>>>>>>>> Â Â Â Â Â Â Â Â * config/arm/arm.h (machine_function): Add
>>>>>>>>> Â Â Â Â Â Â Â Â static_chain_stack_bytes.
>>>>>>>>> Â Â Â Â Â Â Â Â * config/arm/arm.c
>>>>>>>>> (arm_compute_static_chain_stack_bytes):
>>>>>>>>> Â Â Â Â Â Â Â Â Avoid re-computing once computed.
>>>>>>>>> Â Â Â Â Â Â Â Â (arm_expand_prologue): Compute
>>>>>>>>> machine->static_chain_stack_bytes.
>>>>>>>>> Â Â Â Â Â Â Â Â (arm_init_machine_status): Initialize
>>>>>>>>> Â Â Â Â Â Â Â Â machine->static_chain_stack_bytes.
>>>>>>>>>
>>>>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>>>>
>>>>>>>>> 2018-03-21 Sudakshina Das <sudi.das@arm.com>
>>>>>>>>>
>>>>>>>>> Â Â Â Â Â Â Â Â PR target/84826
>>>>>>>>> Â Â Â Â Â Â Â Â * gcc.target/arm/pr84826.c: New test
>>>> The new test fails on
>>>> arm-none-linux-gnueabi
>>>> --with-mode thumb
>>>> --with-cpu cortex-a9
>>>> --with-fpu default
>>>> Dejagnu flags: -march=armv5t
>>>>
>>>> Because:
>>>> /gcc/testsuite/gcc.target/arm/pr84826.c: In function 'a':
>>>> /gcc/testsuite/gcc.target/arm/pr84826.c:15:1: sorry, unimplemented:
>>>> -fstack-check=specific for Thumb-1
>>>> compiler exited with status 1
>>>> FAIL: gcc.target/arm/pr84826.c (test for excess errors)
>>>>
>>>> You probably have to add a require-effective-target to skip the test
>>>> in such cases.
>>>
>>> Yeah, these tests need a
>>> { dg-require-effective-target supports_stack_clash_protection }
>>>
>>> A patch to add that is pre-approved.
>>> Sorry for missing it in review.
>>>
>>> Kyrill
>>>
>>
>> Hi Christophe and Kyrill
>>
>> How about the attached patch?
>> { dg-require-effective-target supports_stack_clash_protection } is not
>> enabled for any of ARM targets, so this is my work around for that.
>> There is
>> a comment in target-supports.exp which makes me a little hesitant in
>> tinkering with the require effective target code.
>>
>> proc check_effective_target_supports_stack_clash_protection { } {
>>
>> Â Â # Temporary until the target bits are fully ACK'd.
>> #Â if { [istarget aarch*-*-*] } {
>> #Â Â Â Â Â Â return 1
>> #Â }
>>
>> Â Â Â if { [istarget x86_64-*-*] || [istarget i?86-*-*]
>> Â Â Â Â Â Â Â Â Â || [istarget powerpc*-*-*] || [istarget rs6000*-*-*]
>> Â Â Â Â Â Â Â Â Â || [istarget s390*-*-*] } {
>> Â Â Â Â Â Â Â return 1
>> Â Â Â }
>> Â return 0
>> }
>>
>> I have opened a new PR 85005 which mentions this that is meant
>> for GCC 9 as part for a bigger cleanup and redesign of the stack
>> clash protection code on ARM backend.
>>
>>
>
> Ok. Thanks for doing this.
Thanks and sorry about this!
Committed as r258805.
Sudi
> Kyrill
>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2018-03-23 Sudakshina Das <sudi.das@arm.com>
>>
>> Â Â Â PR target/84826
>> Â Â Â * gcc.target/arm/pr84826.c: Add dg directive.
>>
>> Thanks
>> Sudi
>>
>>>> Thanks,
>>>>
>>>> Christophe
>>>>
>>>>>>>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>>>>>>>> index bbf3937..2809112 100644
>>>>>>>> --- a/gcc/config/arm/arm.h
>>>>>>>> +++ b/gcc/config/arm/arm.h
>>>>>>>> @@ -1384,6 +1384,9 @@ typedef struct GTY(()) machine_function
>>>>>>>> Â Â Â Â machine_mode thumb1_cc_mode;
>>>>>>>>     /* Set to 1 after arm_reorg has started. */
>>>>>>>> Â Â Â Â int after_arm_reorg;
>>>>>>>> +Â /* The number of bytes used to store the static chain
>>>>>>>> register on the
>>>>>>>> +    stack, above the stack frame. */
>>>>>>>> +Â int static_chain_stack_bytes;
>>>>>>>> Â Â }
>>>>>>>> Â Â machine_function;
>>>>>>>> Â Â #endif
>>>>>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>>>>>> index cb6ab81..bc31810 100644
>>>>>>>> --- a/gcc/config/arm/arm.c
>>>>>>>> +++ b/gcc/config/arm/arm.c
>>>>>>>> @@ -19392,6 +19392,11 @@ arm_r3_live_at_start_p (void)
>>>>>>>> Â Â static int
>>>>>>>> Â Â arm_compute_static_chain_stack_bytes (void)
>>>>>>>> Â Â {
>>>>>>>> +Â /* Once the value is updated from the init value of -1, do not
>>>>>>>> +    re-compute. */
>>>>>>>> +Â if (cfun->machine->static_chain_stack_bytes != -1)
>>>>>>>> +Â Â Â return cfun->machine->static_chain_stack_bytes;
>>>>>>>> +
>>>>>>>>
>>>>>>>> My concern is that this approach caches the first value that is
>>>>>>>> computed
>>>>>>>> for static_chain_stack_bytes.
>>>>>>>> I believe the layout frame code is called multiple times during
>>>>>>>> register
>>>>>>>> allocation as it goes through
>>>>>>>> the motions and I think we want the last value it computes
>>>>>>>> during reload
>>>>>>>>
>>>>>>>> How about we do something like:
>>>>>>>> Â Â Â if (cfun->machine->static_chain_stack_bytes != -1
>>>>>>>> &&epilogue_completed)
>>>>>>>> Â Â Â Â Â return cfun->machine->static_chain_stack_bytes;
>>>>>>>>
>>>>>>>> ?...
>>>>>>>>
>>>>>>>>   /* See the defining assertion in arm_expand_prologue. */
>>>>>>>> Â Â Â Â if (IS_NESTED (arm_current_func_type ())
>>>>>>>> Â Â Â Â Â Â Â Â && ((TARGET_APCS_FRAME && frame_pointer_needed &&
>>>>>>>> TARGET_ARM)
>>>>>>>> @@ -21699,6 +21704,11 @@ arm_expand_prologue (void)
>>>>>>>> Â Â Â Â Â Â Â Â emit_insn (gen_movsi (stack_pointer_rtx, r1));
>>>>>>>> Â Â Â Â Â Â }
>>>>>>>>
>>>>>>>> +Â /* Let's compute the static_chain_stack_bytes required and
>>>>>>>> store it.
>>>>>>>> Right
>>>>>>>> +Â Â Â Â now the value must the -1 as stored by
>>>>>>>> arm_init_machine_status ().
>>>>>>>> */
>>>>>>>>
>>>>>>>> ... this comment would need to be tweaked as
>>>>>>>> cfun->machine->static_chain_stack_bytes may hold
>>>>>>>> an intermediate value computed in reload or some other point before
>>>>>>>> epilogue_completed.
>>>>>>>>
>>>>>>>> +Â cfun->machine->static_chain_stack_bytes
>>>>>>>> +Â Â Â = arm_compute_static_chain_stack_bytes ();
>>>>>>>> +
>>>>>>>
>>>>>>> Maybe I did not understand this completely, but my idea was that
>>>>>>> I am
>>>>>>> initializing the value of cfun->machine->static_chain_stack_bytes
>>>>>>> to be -1
>>>>>>> in arm_init_machine_status () and then it stays as -1 all
>>>>>>> throughout reload
>>>>>>> and hence the function arm_compute_static_chain_stack_bytes ()
>>>>>>> will keep
>>>>>>> computing the value instead of returning the cached value. Only
>>>>>>> during
>>>>>>> expand_prologue (which I assumed occurs much after reload), I
>>>>>>> overwrite the
>>>>>>> initial -1 and after that any call to
>>>>>>> arm_compute_static_chain_stack_bytes
>>>>>>> () would return this cached value.
>>>>>>>
>>>>>>> I did start out writing the patch with a epilogue_completed check
>>>>>>> but
>>>>>>> realized that even during this stage
>>>>>>> arm_compute_static_chain_stack_bytes ()
>>>>>>> was called several times and thus to avoid those re-computations,
>>>>>>> (again
>>>>>>> assuming by this stage we already should have a fixed value) I
>>>>>>> re-wrote it
>>>>>>> with the initialization to -1 approach.
>>>>>>>
>>>>>> Right, I had read the patch too quickly, sorry.
>>>>>> You perform the caching of cfun->machine->static_chain_stack_bytes in
>>>>>> arm_expand_prologue, not arm_compute_static_chain_stack_bytes.
>>>>>> That does give you the right semantics I think.
>>>>>>
>>>>>> The patch is ok then with a small typo fix:
>>>>>
>>>>> Thanks! Committed as r258777.
>>>>>
>>>>> Sudi
>>>>>
>>>>>
>>>>>> +Â /* Let's compute the static_chain_stack_bytes required and
>>>>>> store it.
>>>>>> Right
>>>>>> +Â Â Â Â now the value must the -1 as stored by
>>>>>> arm_init_machine_status ().
>>>>>> */
>>>>>>
>>>>>> s/the/be/
>>>>>>
>>>>>> +Â cfun->machine->static_chain_stack_bytes
>>>>>> +Â Â Â = arm_compute_static_chain_stack_bytes ();
>>>>>> +
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Kyrill
>>>>>>
>>>
>>
>
More information about the Gcc-patches
mailing list