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

[Sudakshina Das]

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

> jeff
>