Closed Bug 870639 Opened 8 years ago Closed 8 years ago

Make Gonk buildable with r11 as a fixed register

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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?
Is skia used in Firefox OS v1.0.1/v1.1 ?
(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.
(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)
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.
jrmuizel, is SkiaGL(Bug 751418) going to be enabled only for 2d canvas on Firefox OS v1.2?
(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.
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)
Keywords: perf
Whiteboard: c=performance → c=
No longer blocks: 810526
Status: NEW → ASSIGNED
Whiteboard: c= → [c=profiling]
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 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)
(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.
(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 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)
For reasons previously described, because even perf with stack copying should work okay at 1kHz.  (Bug 897769)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
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.