Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Description • 11 years ago

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.

	Sotaro Ikeda [:sotaro]
	Comment 1 • 11 years ago

Is skia used in Firefox OS v1.0.1/v1.1 ?

	Jeff Muizelaar [:jrmuizel]
	Comment 2 • 11 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.

	Jeff Muizelaar [:jrmuizel]
	Comment 3 • 11 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?

	Michael Wu [:mwu]
	Comment 4 • 11 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.

	Sotaro Ikeda [:sotaro]
	Comment 5 • 11 years ago

jrmuizel, is SkiaGL(Bug 751418) going to be enabled only for 2d canvas on Firefox OS v1.2?

	Jeff Muizelaar [:jrmuizel]
	Comment 6 • 11 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.

	George Wright (:gw280) (needinfo me!)
	Comment 7 • 11 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.

	Chris Peterson [:cpeterson]
	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. <:)

	George Wright (:gw280) (needinfo me!)
	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

	Timothy B. Terriberry (:derf)
	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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ 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....

	Timothy B. Terriberry (:derf)
	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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 13 • 11 years ago

For reasons previously described, because even perf with stack copying should work okay at 1kHz.  (Bug 897769)