Closed
Bug 870639
Opened 12 years ago
Closed 11 years ago
Make Gonk buildable with r11 as a fixed register
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(b2g18?)
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
b2g18 | ? | --- |
People
(Reporter: jld, Assigned: jld)
References
Details
(Keywords: perf, Whiteboard: [c=profiling s=2013.08.09])
Attachments
(1 file)
external/skia/src/opts/SkBitmapProcState_opts_arm.cpp doesn't compile if r11 is a fixed register — e.g., with the GCC change to get stack frames for profiling (bug 810526 and bug 831611) — because it's clobbered by inline asm that uses all the available registers. The attached patch (against external/skia) fixes that, by having it push/pop the register instead, under an ifdef so the compiled code isn't changed on normal builds. (This means that stacks can't be traced from that code, but we have other places where this is already the case.) I've been applying the patch via a fork of gonk-patches, but I'd like to get it committed so I can reduce the set of unlanded pieces of this profiling work to, at most, the modified toolchain. I gather that we're trying not to add our own changes to gonk-patches, so this would need to a Mozilla-hosted fork of the skia repository. Obviously upstreaming is generally preferred, but I don't know if they'd want this change, and we might need to backport it anyway.
Attachment #747712 -
Flags: review?
Comment 2•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #1) > Is skia used in Firefox OS v1.0.1/v1.1 ? Not yet. But we do still build it.
Comment 3•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2) > (In reply to Sotaro Ikeda [:sotaro] from comment #1) > > Is skia used in Firefox OS v1.0.1/v1.1 ? > > Not yet. But we do still build it. When I first responded I didn't realize that this was external/skia and not the mozilla in-tree version. I'm not sure if we use external/skia or not. We should still try to get something like this in upstream skia. George, can you try to upstream this?
Flags: needinfo?(gwright)
Comment 4•12 years ago
|
||
The mozilla skia isn't enabled on B2G either last I checked - bug 758845 is open for turning it on when we're ready to use it.
Comment 5•12 years ago
|
||
jrmuizel, is SkiaGL(Bug 751418) going to be enabled only for 2d canvas on Firefox OS v1.2?
Comment 6•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #5) > jrmuizel, is SkiaGL(Bug 751418) going to be enabled only for 2d canvas on > Firefox OS v1.2? That is likely.
Comment 7•12 years ago
|
||
I will look into it. I don't have the relevant ARM expertise to properly review this, but please ensure that a .patch file is created for it in gfx/skia/patches. I have CCd reed@google.com on this bug for his input.
Flags: needinfo?(gwright)
Updated•11 years ago
|
Status: NEW → ASSIGNED
Whiteboard: c= → [c=profiling]
Comment 8•11 years ago
|
||
Jed, are you still waiting for someone (reed@google.com?) review your patch? It's been marked r? for two months. <:)
Flags: needinfo?(jld)
Comment 9•11 years ago
|
||
Comment on attachment 747712 [details] [diff] [review] Make external/skia build with fixed r11, if HAVE_APCS_FRAME is defined derf would probably be a suitable reviewer for this
Attachment #747712 -
Flags: review? → review?(tterribe)
Comment 10•11 years ago
|
||
(In reply to George Wright (:gw280) from comment #9) > derf would probably be a suitable reviewer for this What was the actual error generated here? AFAICT, this code doesn't use all the registers. It requires 5 gcc-allocated registers, but 6 (r0-r3,r12,r14) are available. My inclination would just to be to just s/r11/r12/g and add a comment. That will work anywhere that specifies -mapcs-frame or equivalent, instead of relying on build system pieces that aren't actually here and likely can't be upstreamed.
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Timothy B. Terriberry (:derf) from comment #10) > What was the actual error generated here? AFAICT, this code doesn't use all > the registers. It requires 5 gcc-allocated registers, but 6 (r0-r3,r12,r14) > are available. My inclination would just to be to just s/r11/r12/g and add a > comment. That will work anywhere that specifies -mapcs-frame or equivalent, > instead of relying on build system pieces that aren't actually here and > likely can't be upstreamed. That was the first thing I tried. GCC disagrees with the obvious assessment of how many registers there are: external/skia/src/opts/SkBitmapProcState_opts_arm.cpp:107: error: can't find a register in class 'GENERAL_REGS' while reloading 'asm' external/skia/src/opts/SkBitmapProcState_opts_arm.cpp:107: error: 'asm' operand has impossible constraints But at this point we might wind up WONTFIXing this (and several other related bugs), in favor of using the ARM Exception Handling ABI. It might be nice to have a frame pointer option at least as a possibility, but it would be hard to justify landing changes like this (or the compiler modifications, or whatever work IonMonkey would need to not break the frame chains) with a working alternative that follow current standards. Things aren't quite that definite yet, which is (part of) why this bug was in limbo....
Flags: needinfo?(jld)
Comment 12•11 years ago
|
||
Comment on attachment 747712 [details] [diff] [review] Make external/skia build with fixed r11, if HAVE_APCS_FRAME is defined Review of attachment 747712 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jed Davis [:jld] from comment #11) > But at this point we might wind up WONTFIXing this (and several other > related bugs), in favor of using the ARM Exception Handling ABI. It might Okay, clearing the review flag for now, but feel free to re-request if you do decide to go this route, and I can spend some more time looking at why gcc thinks it can't find enough registers.
Attachment #747712 -
Flags: review?(tterribe)
Assignee | ||
Comment 13•11 years ago
|
||
For reasons previously described, because even perf with stack copying should work okay at 1kHz. (Bug 897769)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Updated•11 years ago
|
Whiteboard: [c=profiling] → [c=profiling s=2013.08.09]
You need to log in
before you can comment on or make changes to this bug.
Description
•