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