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

[Jeff Law <law at redhat dot com>]

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.
jeff