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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>>
>>> 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.
For example in emit_store_flag_1 we have this code:
/* If one operand is constant, make it the second one. Only do this
if the other operand is not constant as well. */
if (swap_commutative_operands_p (op0, op1))
{
std::swap (op0, op1);
code = swap_condition (code);
}
if (mode == VOIDmode)
mode = GET_MODE (op0);
I think if you do this in emit_store_flag_force as well everything will
"just work".
You can put it after this call/test pair:
/* First see if emit_store_flag can do the job. */
tem = emit_store_flag (target, code, op0, op1, mode, unsignedp,
normalizep);
if (tem != 0)
return tem;
jeff
