Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi,

On 11 January 2018 at 11:58, Sudakshina Das <sudi.das@arm.com> wrote:
> Hi Jeff
>
>
> On 10/01/18 21:08, Jeff Law wrote:
>>
>> On 01/10/2018 09:25 AM, Sudakshina Das wrote:
>>>
>>> Hi Jeff
>>>
>>> On 10/01/18 10:44, Sudakshina Das wrote:
>>>>
>>>> Hi Jeff
>>>>
>>>> On 09/01/18 23:43, Jeff Law wrote:
>>>>>
>>>>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>>>>>
>>>>>> Hi Jeff
>>>>>>
>>>>>> On 05/01/18 18:44, Jeff Law wrote:
>>>>>>>
>>>>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> The bug reported a particular test di-longlong64-sync-1.c failing
>>>>>>>> when
>>>>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t
>>>>>>>> -O[g,1,2,3]
>>>>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>>>>
>>>>>>>> According to what I could see, the crash was caused because of the
>>>>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force
>>>>>>>> ().
>>>>>>>> Since the comparing argument was a long long, it was being forced
>>>>>>>> into a
>>>>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is
>>>>>>>> done.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag()
>>>>>>>> which
>>>>>>>> says "MODE is the mode to use for OP0 and OP1 should they be
>>>>>>>> CONST_INTs.
>>>>>>>>     If it is VOIDmode, they cannot both be CONST_INT". This
>>>>>>>> condition is
>>>>>>>> not true in this case and thus I think it is suitable to change the
>>>>>>>> argument.
>>>>>>>>
>>>>>>>> Testing done: Checked for regressions on bootstrapped
>>>>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
>>>>>>>> test
>>>>>>>> cases.
>>>>>>>>
>>>>>>>> Sudi
>>>>>>>>
>>>>>>>> ChangeLog entries:
>>>>>>>>
>>>>>>>> *** gcc/ChangeLog ***
>>>>>>>>
>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>
>>>>>>>>        PR target/82096
>>>>>>>>        * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>>>>        to emit_store_flag_force.
>>>>>>>>
>>>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>>>
>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>
>>>>>>>>        PR target/82096
>>>>>>>>        * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>>>>        * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>>>>>
>>>>>>> In the case where both (op0/op1) to
>>>>>>> emit_store_flag/emit_store_flag_force are constants, don't we know
>>>>>>> the
>>>>>>> result of the comparison and shouldn't we have optimized the store
>>>>>>> flag
>>>>>>> to something simpler?
>>>>>>>
>>>>>>> I feel like I must be missing something here.
>>>>>>>
>>>>>>
>>>>>> emit_store_flag_force () is comparing a register to op0.
>>>>>
>>>>> ?
>>>>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>>>>      and storing in TARGET.  Normally return TARGET.
>>>>>      Return 0 if that cannot be done.
>>>>>
>>>>>      MODE is the mode to use for OP0 and OP1 should they be
>>>>> CONST_INTs.  If
>>>>>      it is VOIDmode, they cannot both be CONST_INT.
>>>>>
>>>>>
>>>>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>>>>> CONST_INT.  If both are a CONST_INT, then you need to address the
>>>>> problem in the caller (by optimizing away the condition).  If you've
>>>>> got
>>>>> a REG and a CONST_INT, then the mode should be taken from the REG
>>>>> operand.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>>>>> function. emit_store_flag_force () is used in case when this
>>>>>> function is
>>>>>> called by the bool variant of the built-in function where the bool
>>>>>> return value is computed by comparing the result register with the
>>>>>> expected op0.
>>>>>
>>>>> So if only one of the two objects is a CONST_INT, then the mode should
>>>>> come from the other object.  I think that's the fundamental problem
>>>>> here
>>>>> and that you're just papering over it by changing the caller.
>>>>>
>>>> I think my earlier explanation was a bit misleading and I may have
>>>> rushed into quoting the comment about both operands being const for
>>>> emit_store_flag_force(). The problem is with the function and I do
>>>> agree with your suggestion of changing the function to add the code
>>>> below to be a better approach than the changing the caller. I will
>>>> change the patch and test it.
>>>>
>>>
>>> This is the updated patch according to your suggestions.
>>>
>>> Testing: Checked for regressions on arm-none-linux-gnueabihf and added
>>> new test case.
>>>
>>> Thanks
>>> Sudi
>>>
>>> ChangeLog entries:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>      PR target/82096
>>>      * expmed.c (emit_store_flag_force): Swap if const op0
>>>      and change VOIDmode to mode of op0.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>      PR target/82096
>>>      * gcc.c-torture/compile/pr82096.c: New test.
>>
>> OK.
>
>
> Thanks. Committed as r256526.
> Sudi
>

Could you add a guard like in other tests to skip it if the user added
-mfloat-abi=XXX when running the tests?

For instance, I have a configuration where I add
-mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
and the new test fails because:
xgcc: error: -mfloat-abi=soft and -mfloat-abi=hard may not be used together

Thanks

Christophe

>> jeff
>>
>