https://codereview.chromium.org/16109010/diff/1/src/atomicops_internals_arm_gcc.h
File src/atomicops_internals_arm_gcc.h (right):
https://codereview.chromium.org/16109010/diff/1/src/atomicops_internals_arm_gcc.h#newcode96
src/atomicops_internals_arm_gcc.h:96: " teq %0, %4\n"
On 2013/06/12 10:44:48, digit1 wrote:
On 2013/06/11 16:12:52, Rodolph Perfetta wrote:
> cmp %0, %4 is more readable here.
As the original version of this file, this is a straight copy of the
sources
that have been already submitted to the Chromium
base/atomicops_internal_arm_gcc.h header.
I'd like to keep them identical if possible. Would keeping the teq
here seems
acceptable to you? Alternatively, I could fix the base/ version first,
and would
gladly add you as a reviewer :-)
(Note: I also need to port this to protobuf which uses yet another
copy).
I am happy to review the original patch. Not only cmp is more readable
but it is better as it set all the flags when teq only set NZ, making
life harder for modern CPUs.
https://codereview.chromium.org/16109010/diff/1/src/atomicops_internals_arm_gcc.h#newcode104
src/atomicops_internals_arm_gcc.h:104: } while (reloop != 0);
On 2013/06/12 10:44:48, digit1 wrote:
On 2013/06/11 16:12:52, Rodolph Perfetta wrote:
> You didn't execute clrex when the cmp failed (not mandatory but nice
to have)
I'm really curious to know why this would be "nice to have", do you
have details
to share? :-)
My understanding is that clrex is unnecessary on Linux, because the
kernel
always uses it on thread switches. This takes care of the problem that
STREX can
only detect concurrent writes performed by other cores, but not the
current one.
Disregard that comment, it only helps broken code. You are right the
kernel will do all the necessary work.
https://codereview.chromium.org/16109010/diff/1/src/atomicops_internals_arm_gcc.h#newcode161
src/atomicops_internals_arm_gcc.h:161: // fail = STREX(ptr, new_value)
On 2013/06/12 10:44:48, digit1 wrote:
On 2013/06/11 16:12:52, Rodolph Perfetta wrote:
> nit: reloop = STREX(ptr, new_value)
Damn, yes. Same concern as above. Is it worth modifying the file?
If you submit a new patch for cmp then yes.
https://codereview.chromium.org/16109010/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
