On 01/12/2018 01:45 AM, Christophe Lyon wrote: > 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 It's starting to feel like the test should move into gcc.target/arm :-) I nearly suggested that already. Consider moving it into gcc.target/arm pre-approved along with adding the -O<whatever you need> to the options and whatever is needed to skip the test at the appropriate time.
jeff
