This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 02/20/2014 01:22 PM, Richard Henderson wrote: > On 02/20/2014 12:09 PM, Jakub Jelinek wrote: >> On Thu, Feb 20, 2014 at 11:49:30AM -0600, Richard Henderson wrote: >>> Tested on x86_64 and i686, and manually inspecting the generated code. >>> Any ideas how to regression test this? >> >> No idea about how to test this. >> >>> @@ -5330,14 +5330,23 @@ expand_builtin_atomic_compare_exchange (enum machine_mode mode, tree exp, >>> if (tree_fits_shwi_p (weak) && tree_to_shwi (weak) != 0) >>> is_weak = true; >>> >>> + if (target == const0_rtx) >>> + target = NULL; >>> oldval = expect; >>> - if (!expand_atomic_compare_and_swap ((target == const0_rtx ? NULL : &target), >>> - &oldval, mem, oldval, desired, >>> + >>> + if (!expand_atomic_compare_and_swap (&target, &oldval, mem, oldval, desired, >> >> I'm wondering if this shouldn't be instead: >> oldval = NULL; >> if (!expand_atomic_compare_and_swap (&target, &oldval, mem, expected, desired, >> is_weak, success, failure)) >> >> because otherwise expand_atomic_compare_and_swap could in theory already >> store into expect MEM, couldn't it? I mean, it does: >> /* Load expected into a register for the compare and swap. */ >> if (MEM_P (expected)) >> expected = copy_to_reg (expected); >> >> /* Make sure we always have some place to put the return oldval. >> Further, make sure that place is distinct from the input expected, >> just in case we need that path down below. */ >> if (ptarget_oval == NULL >> || (target_oval = *ptarget_oval) == NULL >> || reg_overlap_mentioned_p (expected, target_oval)) >> target_oval = gen_reg_rtx (mode); >> so with NULL *ptarget_oval it will surely allocate a pseudo, but if it is >> the expected MEM, as expected has been forced into register earlier, >> I don't think it overlaps with that REG and thus it can be already stored >> and have oldval == expect after the call. > > I don't know any target that actually accepts a MEM for oldval, and since the > current definition of __atomic_compare_and_swap_n takes an address for > expected, we'll always have a MEM. So at present we'll always allocate a new > pseudo just as if we zero out oldval. > > But, fair enough. It does seem generally safer your way. Like so. r~
Attachment:
d-60272-2
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |