Reviewers: danno, ulan, Rodolph Perfetta,
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/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).
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/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.
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/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?
https://codereview.chromium.org/16109010/diff/1/src/atomicops_internals_arm_gcc.h#newcode172
src/atomicops_internals_arm_gcc.h:172: #elif defined(__ARM_ARCH_5__) ||
defined(__ARM_ARCH_5T__) || \
On 2013/06/11 16:12:52, Rodolph Perfetta wrote:
V8 requires ARMv6 as a baseline, you don't need this code.
Ah, we could definitely remove this from base/ then. Looks like I'll be
preparing a new patch then.
https://codereview.chromium.org/16109010/diff/1/src/atomicops_internals_arm_gcc.h#newcode260
src/atomicops_internals_arm_gcc.h:260: // which always implement a full
barrier.
On 2013/06/11 14:48:23, ulan wrote:
If NoBarrier_CompareAndSwap returns early because of (prev_value !=
old_value)
then we will not emit barrier. Is this correct because we care only
about the
store operation not being reordered?
That's a good question. I assumed as you say that only the store
matters, but the semantics of these operations are poorly explained in
atomicops.h anyway, so a caller could think otherwise.
It looks like this code is going to be removed, and the ARMv6+ version
always implement the barrier, so in the end that probably doesn't
matter.
Description:
Improve Linux/ARM atomic operations for ARMv6 and higher.
This is a port of the following Chromium patch:
https://chromiumcodereview.appspot.com/16335007/
In a nutshell, using LDREX/STREX directly allows for faster
atomic operations, especially for those that don't need
memory barriers. See the comment in the original patch for
more details.
+ Reformat src/mips/assembler-mips.cc to pass the C++ lint
presubmit checks.
BUG=2718
[email protected]
Please review this at https://codereview.chromium.org/16109010/
SVN Base: git://github.com/v8/v8.git@master
Affected files:
M src/atomicops_internals_arm_gcc.h
M src/mips/assembler-mips.cc
--
--
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.
