956606 - error: 'GetFloatArgStackDisp' was not declared in this scope

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine: JIT, defect)

Description

[Oleg Romashin (:romaxa)]

Sounds like in this changeset http://hg.mozilla.org/mozilla-central/rev/d3cb4aa974a7 
we renamed GetFloatArgStackDisp to Double but in place where it is used

mJSLoadHeap(js::jit::MAsmJSLoadHeap*)':
js/src/jit/arm/Lowering-arm.cpp:491:17: warning: unused variable 'ptrValue' [-Wunused-variable]
In file included from js/src/Unified_cpp_5.cpp:92:0:
js/src/jit/arm/MacroAssembler-arm.cpp: In member function 'void js::jit::MacroAssemblerARMCompat::passABIArg(const js::jit::MoveOperand&, js::jit::MoveOp::Type)':
js/src/jit/arm/MacroAssembler-arm.cpp:3536:91: error: 'GetFloatArgStackDisp' was not declared in this scope
make[1]: *** [Unified_cpp_5.o] Error 1

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: GetFloatArgStackDisp > GetDoubleArgStackDisp

Comment 2

[Dan Gohman [:sunfish]]

Comment on attachment 8355933 [details] [diff] [review]
GetFloatArgStackDisp > GetDoubleArgStackDisp

This is in code used for both MoveOp::DOUBLE and MoveOp::FLOAT32. Should this be using GetFloat32ArgStackDisp for the FLOAT32 case? If not, then perhaps the GetFloat32ArgStackDisp function added in bug 949668 isn't actually needed.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8355933 [details] [diff] [review]
GetFloatArgStackDisp > GetDoubleArgStackDisp

Forwarding to sunfish.

Comment 4

[Dan Gohman [:sunfish]]

Marty, do you know how much stack space to float32 arguments should get in the ARM hardfp ABI?

Comment 5

[Marty Rosenberg [:mjrosenb]]

They use 32 bits.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: hardfp-arg-stack-reg.patch

In that case, the attached patch is probably what is needed. However, I'm not currently able to test this, on a hardfp ARM platform, so it's currently untested.

Comment 7

[Oleg Romashin (:romaxa)]

> In that case, the attached patch is probably what is needed. However, I'm
> not currently able to test this, on a hardfp ARM platform, so it's currently
> untested.
Cannot test it properly because, of bug 957504

Comment 8

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8357241 [details] [diff] [review]
hardfp-arg-stack-reg.patch

Review of attachment 8357241 [details] [diff] [review]:
-----------------------------------------------------------------

This patch isn't wrong, but it is not correct either.  This code path needs a large amount of reworking.  I'm currently about 5 yaks deep in it.

Comment 9

[Oleg Romashin (:romaxa)]

> a large amount of reworking.  I'm currently about 5 yaks deep in it.

Is there are chance to get it fixed in 29.0 ?

Comment 10

[Douglas Crosher [:dougc]]

If this path is only needed for calls to system maths functions then perhaps a temporary workaround would be to just disable the specialization to float32 ops on the ARM hardfp build of functions that pass more than one float32 argument.  There appears to be a similar workaround in the Odin compiler for pow and atan2.  Perhaps fixing this depends on bug 962263.

Comment 11

[Douglas Crosher [:dougc]]

The ARM JIT simulator in bug 959597 adds code that accounts for every combination of passed arguments for ABI calls, and interestingly the only combination needed for the simulator with float32 arguments is the Args_Float32_Float32 case with a single float32 argument. So, perhaps this code path is not even being used, perhaps just assert that usedIntSlots_ and usedFloatSlots_ are zero for a float32 argument to get this building again, and keep a look out for assertion failures?

Comment 12

[Oleg Romashin (:romaxa)]

This I beleive already fixed