Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
450343002:
Adding atomic ops for MIPS64. (Closed)
DescriptionAdding atomic ops for MIPS64.
This change adds atomic operations for MIPS64.
BUG=400684
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289624
Patch Set 1 #
Total comments: 6
Patch Set 2 : Upload correct MIPS64 related change #
Total comments: 9
Patch Set 3 : Update per code review. #
Messages
Total messages: 17 (0 generated)
Please take a look.
s/me/will/
NACK. Extra platforms add a lot of burden to the project and Chrome is already stretching to handle a great many of them. Essentially none of the developers will have access to this hardware nor will be able to do anything if it breaks etc. I'm afraid that adding extra platforms is a much larger change than it appears from the weight of the diff. Chrome is, of course, open source, and you can maintain platform specific versions at will outside of the main project.
On 2014/08/08 17:30:12, agl wrote: > NACK. > > Extra platforms add a lot of burden to the project and Chrome is already > stretching to handle a great many of them. Essentially none of the developers > will have access to this hardware nor will be able to do anything if it breaks > etc. I'm afraid that adding extra platforms is a much larger change than it > appears from the weight of the diff. > > Chrome is, of course, open source, and you can maintain platform specific > versions at will outside of the main project. Hello Adam, MIPS32 is already a supported architecture in Chromium both for Linux and Android OS. Number of buildbots are watching the state of it including a buildbot [1] at Google's side. Number of developers are fixing issues when they pop up. MIPS64 port of Chromium is already done. Patches for v8 have been completed and upstreamed [2], and we are in process of upstreaming very small number of Chromium patches needed for MIPS64. You can see the list of already accepted patches at the MIPS64 Chromium tracking page [3]. This patch for atomic ops is one of the last few patches needed for the completion of this task. Support for MIPS64 in Android OS is being upstreamed as we speak (I can point you to a lengthy list of MIPS64 specific patches if needed), so there is a clear target for Chrome Android for MIPS64. Android pulls the code from this repository, so it only makes sense to have all the code in this place. I do understand the challenges of having multiple platforms supported, and thus we do a lot to help. We have been doing it for two last years for MIPS32, and we will continue to do the same for MIPS64 as well. Kind Regards, Petar Jovanovic [1] Android MIPS Builder (dbg), http://build.chromium.org/p/chromium.fyi/builders/Android%20MIPS%20Builder%20... [2] v8 MIPS64 port, https://codereview.chromium.org/371923006/ [3] Chrome/Android on MIPS64 tracking bug, https://code.google.com/p/chromium/issues/detail?id=400684
On 2014/08/08 18:20:28, petarj wrote: > MIPS64 port of Chromium is already done. Patches for v8 have been > completed and upstreamed [2], and we are in process of upstreaming very > small number of Chromium patches needed for MIPS64. You can see the list > of already accepted patches at the MIPS64 Chromium tracking page [3]. > This patch for atomic ops is one of the last few patches needed for > the completion of this task. The referenced bug was filed a couple of days ago by yourselves, it's hardly a decision to support the platform :) The other reviewers may have just reviewed the code for correctness. None the less, a new platform is not something that we do lightly. It might well be something that we want, but you need to make the case on chromium-dev and the project needs to make a decision. Commitments like this shouldn't happen in the narrow scope of a code review. I don't think that we can figure out whether this change is the right way to implement MIPS64 until we decide whether it's the right thing to do at all.
I've asked around and it does appear that we will be supporting MIPS64 so this review can continue. https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... base/atomicops_internals_mips_gcc.h:32: "ll %0, 0(%6)\n" // prev = *ptr It seems that the old MIPS code was always broken here? The linked load was loading from a register thus never did anything? Is that the reason for the change? However, with this change, %1 and %5 are never used thus should be removed from the arguments section. https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... base/atomicops_internals_mips_gcc.h:41: : "Ir" (old_value), "r" (new_value), "m" (*ptr), "r" (ptr) please wrap to 80 chars. https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... base/atomicops_internals_mips_gcc.h:54: "ll %1, 0(%5)\n" // old = *ptr %2 is now unused. (And %4 was always unused?) https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... base/atomicops_internals_mips_gcc.h:76: "ll %0, 0(%5)\n" // temp = *ptr %2 is now unused. (And, again, %4 was never used?) https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... base/atomicops_internals_mips_gcc.h:80: "addu %1, %0, %3\n" // temp2 = temp + increment I'm assuming that this is filling a delay slot which is the reason for doing it this odd way around? https://codereview.chromium.org/450343002/diff/1/base/atomicops_internals_mip... base/atomicops_internals_mips_gcc.h:161: "lld %0, 0(%6)\n" // prev = *ptr These appear to have been copied from the 32 bit ones and thus have the save superfluous input and output arguments. Please make the same changes here and in the two other blocks, below.
My apologies, I have accidentally uploaded a version with a temp workaround for a gcc issue for r6 ISA. Patch Set 2 is the change that adds MIPS64 atomic operations. Please take a look.
https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:161: "lld %0, %5\n" // prev = *ptr Can you post a disassembly of this function? (objdump -d on the .o file and trim, for example.) %5 is asking for the memory location of *ptr. I've checked on x86-64 and GCC uses |ptr| for this. However, I think it would be completely valid for it to make a copy of *ptr and use the location of the copy. Why wouldn't you have %5 be "r" (ptr) and then "lld %0, (%5)" to eliminate this? Also, it appears that the first argument is the destination in this asm. Which is backwards from the usual AT&T syntax! (And AT&T syntax was supposed to be a "universal" asm syntax framework.) However, if this compiles then I'm assuming that it must be correct.
objdump added. https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:161: "lld %0, %5\n" // prev = *ptr On 2014/08/12 18:14:22, agl wrote: > Can you post a disassembly of this function? (objdump -d on the .o file and > trim, for example.) > > %5 is asking for the memory location of *ptr. I've checked on x86-64 and GCC > uses |ptr| for this. However, I think it would be completely valid for it to > make a copy of *ptr and use the location of the copy. Why wouldn't you have %5 > be "r" (ptr) and then "lld %0, (%5)" to eliminate this? > > Also, it appears that the first argument is the destination in this asm. Which > is backwards from the usual AT&T syntax! (And AT&T syntax was supposed to be a > "universal" asm syntax framework.) However, if this compiles then I'm assuming > that it must be correct. Here it is: 0000000000000000 <NoBarrier_CompareAndSwap(int volatile*, int, int)>: 0: 67bdffd0 daddiu sp,sp,-48 4: ffbe0028 sd s8,40(sp) 8: 03a0f02d move s8,sp c: ffc40010 sd a0,16(s8) 10: 00a0182d move v1,a1 14: 00c0102d move v0,a2 18: 00031800 sll v1,v1,0x0 1c: afc30018 sw v1,24(s8) 20: 00021000 sll v0,v0,0x0 24: afc2001c sw v0,28(s8) 28: dfc60010 ld a2,16(s8) 2c: 8fc20018 lw v0,24(s8) 30: 8fc3001c lw v1,28(s8) 34: dfc40010 ld a0,16(s8) 38: 7c850037 lld a1,0(a0) 3c: 14a20000 bne a1,v0,40 <NoBarrier_CompareAndSwap(int volatile*, int, int)+0x40> 40: 0060382d move a3,v1 44: 7cc70027 scd a3,0(a2) 48: 10e00000 beqz a3,4c <NoBarrier_CompareAndSwap(int volatile*, int, int)+0x4c> 4c: 00000000 nop 50: 00e0202d move a0,a3 54: afc50000 sw a1,0(s8) 58: afc40004 sw a0,4(s8) 5c: 8fc20000 lw v0,0(s8) 60: 03c0e82d move sp,s8 64: dfbe0028 ld s8,40(sp) 68: 67bd0030 daddiu sp,sp,48 6c: 03e00009 jr ra 70: 00000000 nop
https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:161: "lld %0, %5\n" // prev = *ptr On 2014/08/12 20:30:22, gordanac wrote: > Here it is: > 0000000000000000 <NoBarrier_CompareAndSwap(int volatile*, int, int)>: > 0: 67bdffd0 daddiu sp,sp,-48 > 4: ffbe0028 sd s8,40(sp) > 8: 03a0f02d move s8,sp > c: ffc40010 sd a0,16(s8) > 10: 00a0182d move v1,a1 > 14: 00c0102d move v0,a2 > 18: 00031800 sll v1,v1,0x0 > 1c: afc30018 sw v1,24(s8) > 20: 00021000 sll v0,v0,0x0 > 24: afc2001c sw v0,28(s8) > 28: dfc60010 ld a2,16(s8) > 2c: 8fc20018 lw v0,24(s8) > 30: 8fc3001c lw v1,28(s8) > 34: dfc40010 ld a0,16(s8) > 38: 7c850037 lld a1,0(a0) > 3c: 14a20000 bne a1,v0,40 <NoBarrier_CompareAndSwap(int > volatile*, int, int)+0x40> That seems sane (although not optimised). But don't you agree that asking for "m" (*ptr) in order to get ptr is weird and probably error prone? It already has a memory clobber.
https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:161: "lld %0, %5\n" // prev = *ptr On 2014/08/12 21:05:49, agl wrote: > That seems sane (although not optimised). But don't you agree that asking for > "m" (*ptr) in order to get ptr is weird and probably error prone? It already has > a memory clobber. This function gets inlined, so the code is more optimal at places where it is used. As of your question of "lld %0, %5", AFAICS glibc does it [1] the same way, so I'd vote we keep it in that fashion. Regards, Petar [1] glibc, MIPS atomics https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/mips/bits/atom...
LGTM with nits. https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:161: "lld %0, %5\n" // prev = *ptr On 2014/08/12 23:40:40, petarj wrote: > As of your question of "lld %0, %5", AFAICS glibc does it [1] the same > way, so I'd vote we keep it in that fashion. Fair enough. glibc doing it should mean that any compiler change that breaks it gets noticed before we hit it. https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:183: "lld %1, %2\n" // old = *ptr The %2 here is an output argument being used as an input to lld, right? That seems wrong (and the glibc sources for the same use an input argument here.) This has been duplicated from the version above so please fix there too if you change this. https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:205: "lld %0, %2\n" // temp = *ptr ditto.
lgtm rubberstamp for base/OWNERS
https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... File base/atomicops_internals_mips_gcc.h (right): https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:183: "lld %1, %2\n" // old = *ptr On 2014/08/13 17:52:37, agl wrote: > The %2 here is an output argument being used as an input to lld, right? That > seems wrong (and the glibc sources for the same use an input argument here.) > > This has been duplicated from the version above so please fix there too if you > change this. Done. https://codereview.chromium.org/450343002/diff/20001/base/atomicops_internals... base/atomicops_internals_mips_gcc.h:205: "lld %0, %2\n" // temp = *ptr On 2014/08/13 17:52:37, agl wrote: > ditto. Done.
The CQ bit was checked by gordana.cmiljanovic@imgtec.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Gordana.Cmiljanovic@imgtec.com/4503430...
Message was sent while issue was closed.
Committed patchset #3 (40001) as 289624 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
