Recent changes to improve the typechecker in nanojit appear to cause bogus Validate errors when softfloat is used. Specifically, we get LIR type error in instruction with opcode callh... we're getting an int but expecting a float64. (Presumably the typechecker needs to be told about the secret int/float dichotomy for softfloat)

(In reply to comment #0) > (Presumably the typechecker needs to be told about the secret int/float > dichotomy for softfloat) Presumably the author of the typechecker needs to be told as well! Is this related to https://bugzilla.mozilla.org/show_bug.cgi?id=541491#c50 ? The comment I added about callh in LIRopcode.tbl is probably wrong too. I thought I understood callh but clearly I don't. I've started working on bug 530754, which will run lirasm tests on all relevant platform combinations with 'make check', and will help prevent these problems. But these assertions need to be fixed before we land that.

Yeah, this is a bit of a nebulous bug -- apologies for that, it just came up in some Flash integration work and I haven't had time to figure out what's what yet. (I think it's just a bogus assertion, but it may be a legitimate bug -- not sure yet.) Apologies for the vagueness -- I'll update with more concrete specifics as soon as I can.

I think what's going on is that ValidateWriter is (incorrectly) requiring the argument of LIR_callh to be LIR_fcall; that's what it was originally, but insCall() transmuted it to LIR_icall by this point, so that's what we should check for.

FWIW, the patch causes the assertions to go away, but SoftFloat still seems to be generally broken. (Sorry, I don't have specifics on it yet; I'll open a new bug for that once I have them.)

Comment on attachment 424453 [details] [diff] [review] Patch >diff -r de2ddd123317 nanojit/LIR.cpp >--- a/nanojit/LIR.cpp Sat Jan 30 17:30:59 2010 -0800 >+++ b/nanojit/LIR.cpp Sat Jan 30 18:55:29 2010 -0800 >@@ -2555,7 +2555,10 @@ > break; > > case LIR_callh: >- checkLInsHasOpcode(op, 1, a, LIR_fcall); >+ // This looks odd, but is right: if op==LIR_callh, >+ // the *original* call was LIR_fcall, but insCall() >+ // transmuted it into LIR_icall. That's not right -- you don't specify an opcode to insCall(), it determines one itself once it reaches LirBufWriter::insCall(). So it doesn't make sense to say "the original call was LIR_fcall". >+ checkLInsHasOpcode(op, 1, a, LIR_icall); > formals[0] = LTy_F64; > break; Shouldn't formals[0] by LTy_I32 if the argument is a LIR_icall? Also, the comment above callh in LIRopcode.tbl is wrong and should be corrected as part of this bug. But I still haven't worked out what it's really supposed to say. Can someone please explain to me how callh is supposed to work?

(In reply to comment #4) > FWIW, the patch causes the assertions to go away, but SoftFloat still seems to > be generally broken. (Sorry, I don't have specifics on it yet; I'll open a new > bug for that once I have them.) Fixing that will require enabling testing of SoftFloat in lirasm. I'm working on part of that (enabling multiple configurations) in bug 530754. But a SoftFloatFilter implementation will have to be added to lirasm as well.

(Steven asked me offline to expand on the meaning of LIR_callh) the intended semantics of LIR_callh is to return the upper 32bits of a 64-bit result of a function call. callh's input is the call itself. for example, an arm call that returns a result in R0:R1 should have R0 assigned to the call (LIR_icall, i guess), and R1 assigned to the LIR_callh. LIR_callh takes the LIR_icall as "input" mainly to maintain a data-flow link between the two LIR instructions so they don't necessarily have to be adjacent. For example if the result of LIR_icall is never used, but the result of LIR_callh is used, then the call isn't dead.

Ok, let's assume we have a call on ARM machine lacking VFP: insCall(f_ci, args) where f_ci is a CallInfo for a function that returns a float (double). In SoftFloatFilter::insCall() we'll split each of the args, and then do this: LIns *lo = out->insCall(f_ci, args); LIns *hi = out->ins1(LIR_callh, lo); return out->ins2(LIR_qjoin, lo, hi); In LirBufWriter::insCall() we'll select LIR_fcall for the op, then change it to LIR_callh, then change it to LIR_icall (yes, really). The net result is this code: lo = icall(f_ci, args) hi = callh(lo) res = qjoin(lo, callh(lo)) But the debug printing of qjoin/callh is broken so I can't confirm that, I get total rubbish sequences when I try --novfp. I think the debug printing is trying to be clever with qjoin/callh and just making things worse. The trouble with SoftFloat is that it's broken in about 5 different ways. But here's a patch that has similar scope to stejohns' original. It: - Fixes the type-checking of LIR_callh. - Fixes the incorrect comment in LIRopcode.tbl about LIR_callh. - In LirBufWriter()::insCall() for SoftFloat it no longer converts fcall to callh and then to icall, it just converts fcall directly to icall.

Comment on attachment 424686 [details] [diff] [review] alternative patch Patch looks good to me. Adding dschaffe for review to ensure that it's been tested on an actual ARM device with softfloat enabled.

Comment on attachment 424453 [details] [diff] [review] Patch withdrawing review request; I like Nick's patch better.

Comment on attachment 424686 [details] [diff] [review] alternative patch I tested this patch on windows mobile with and without softfloat and it passed the TR acceptance suite. There are a couple of failures (5) that I will log bugs for but I am confident they are not related to this issue and just bugs that have not been noticed before.

Comment on attachment 424686 [details] [diff] [review] alternative patch confirmed on android patch fixes LIR assertion.

Thanks for the reviews and testing. http://hg.mozilla.org/projects/nanojit-central/rev/7d2cd297ad03