Hi, On 11 January 2018 at 11:58, Sudakshina Das <sudi....@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....@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....@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....@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....@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 >> >