499664 - TM: eliminate 64-bit LIR_callh hack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

On 64-bit platforms we treated callh like fcall/call but on 32-bit platforms its a unary operation that returns the hi-word of a call. This is inconsistent and confusing. We can push this functionality into the backend. Restore the original unary form of callh for all platforms. This is particularly easy since we don't have a 64-bit backend at the moment.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 384381 [details] [diff] [review]
patch

> 	bool LIns::isQuad() const {
>-#ifdef AVMPLUS_64BIT
>-		// callh in 64bit cpu's means a call that returns an int64 in a single register
>-		return (firstWord.code & LIR64) != 0 || firstWord.code == LIR_callh;
>-#else
>-		// callh in 32bit cpu's means the 32bit MSW of an int64 result in 2 registers
> 		return (firstWord.code & LIR64) != 0;
>-#endif

You can remove the "!= 0".


>     LIns* LirBufWriter::insCall(const CallInfo *ci, LInsp args[])
> 	{
>-		static const LOpcode k_callmap[]  = { LIR_call,  LIR_fcall,  LIR_call,  LIR_callh };
>-
>-		uint32_t argt = ci->_argtypes;
>-        LOpcode op = k_callmap[argt & 3];
>-        NanoAssert(op != LIR_skip); // LIR_skip here is just an error condition
>+		// Use LIR_fcall only when using the FPU.
>+		LOpcode op = (!AvmCore::config.soft_float && ((ci->_argtypes & 3) == ARGSIZE_F))
>+			? LIR_fcall : LIR_call;
> 
>         ArgSize sizes[MAXARGS];
>         int32_t argc = ci->get_sizes(sizes);
> 
>-		if (AvmCore::config.soft_float) {
>-			if (op == LIR_fcall)
>-				op = LIR_callh;
>-		}

I'm really unsure about this.  Partly because I don't really understand the original version (why does it only check the low two bits of ci->_argtypes?  presumably that's the first arg of the call), and partly because I don't know if you're trying to preserve its behaviour or not (you don't, because LIR_callh is never produced).


Other than that, the patch looks fine, in the sense that it changes callh to have a single meaning.  But I don't like that it's not tested -- AFAICT callh only is generated in soft_float mode, which is off by default, and last time I checked soft_float was totally broken.  Presumably after this change it will still be broken, and possibly in a different way.  When do/will we use soft_float?


Furthermore, the more I think about it, the more confused I am about the semantics of callh.  The description is that it "accesses the high 32 bits of a call returning a 64-bit result as a pair of 32-bit values".  Here's how it's used by SoftFloatFilter (the only code that generates, AFAICT):

        lo = call(...args...)
        hi = callh(lo)
        return qjoin(lo, hi)

The meaning of this is very unclear.  'lo' must be a 32-bit value, and 'callh(lo)' does not mean "perform an operation on 'lo'" but rather "get the 'hi' value that corresponds to 'lo' and was magically stashed somewhere".  It is a semantically dodgy instruction of the sort Julian complained about in bug 495569.  Surely there's a cleaner way to do this?  Since callh and qjoin are only ever used together, AFAICT, maybe we should remove them and add a new call opcode that represents the code snippet above.  We could have clean semantics and one fewer opcodes.


As for the lack of x86-64 backend making it easier, yes it makes this change easier now, but the x86-64 integration will be more difficult as the integrator will have to be aware of the change and handle it accordingly (unless it's applied to Tamarin first).

That's a longer review than you were expecting, sorry... I don't want to grant review until the all above stuff is clarified.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

Pushed to TM (only good way to test on ARM).

http://hg.mozilla.org/tracemonkey/rev/55a8910d8436

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> Pushed to TM (only good way to test on ARM).
> 
> http://hg.mozilla.org/tracemonkey/rev/55a8910d8436

That's certainly not the only way to test on ARM.  I have an ARM emulator set up, as do numerous other people.

Also, I didn't realise it was kosher, if a patch doesn't pass review, to rebase it, repost it, and ask someone else to review it.  Cute.  I'll be sure to remember that next time someone rejects one of my patches.

I'm not impressed.

Comment 6

[Andreas Gal :gal]

Debugging some other bug atm, will respond in a sec. Backed out patch again until matter is resolved.

http://hg.mozilla.org/tracemonkey/rev/a8fafd6b563e

Comment 7

[Andreas Gal :gal]

I apologize for the confusion. I didn't mean to shortcut your review. Our discussion yesterday ended with "if I know that soft_float works then I think I'll be able to r+, after I check the insCall stuff again with my newfound knowledge of nanojit arcania". I understood that as "patch is ok, except for knowing whether softfloat still works". If it was meant to say something else, I am sorry for misunderstanding what you meant.

I asked vlad about the status of our test coverage for ARM and turns out the tinderbox is still in a box. I didn't know that when I pushed. The intent was to get a quick feedback from the ARM tinderbox whether the patch is ok or not.

Independently of the testing angle, I discussed the callh change with vlad (who did the ARM code for it), and dvander (who introduced the changes I am undoing). You said you weren't sure about the change, so I looked for people who should be. I checked for you on irc but you weren't around. 2 other qualified people were and I wanted to get this out of my queue, so I pushed it. I don't think its the end of the world and patches can be backed out easily.

What do you want me to do with the patch? Put you up for review and wait until our ARM test coverage is back online?

Comment 8

[Graydon Hoare :graydon]

FYI I'm now quite on-side with Julian about the usability of the ubunty ARM system qemu. Now that I have it mounting NFS from my workstation, it's exactly as usable as the scratchbox, but with nowhere near the setup complexity. Basically you just need a linux machine with a trunk qemu build. Add an NFS server if you want to access your host filesystem to test the same source workspace you're testing outside the box. That's it.

I can post the system image as a tarball if you like. It's really easy.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> I apologize for the confusion. I didn't mean to shortcut your review. Our
> discussion yesterday ended with "if I know that soft_float works then I think
> I'll be able to r+, after I check the insCall stuff again with my newfound
> knowledge of nanojit arcania". I understood that as "patch is ok, except for
> knowing whether softfloat still works". If it was meant to say something else,
> I am sorry for misunderstanding what you meant.

My comment had two conditions:
- knowing that soft_float was working
- "after I check..."

Neither of those were met.

> I asked vlad about the status of our test coverage for ARM and turns out the
> tinderbox is still in a box. I didn't know that when I pushed. The intent was
> to get a quick feedback from the ARM tinderbox whether the patch is ok or not.

I understand the intent, but I don't consider it a legitimate way to test.  What if everybody started pushing to TM just to test?  It would be a disaster.  That's why we have try servers and why many people have gone to the effort of setting up ARM VMs.

> Independently of the testing angle, I discussed the callh change with vlad (who
> did the ARM code for it), and dvander (who introduced the changes I am
> undoing). You said you weren't sure about the change, so I looked for people
> who should be. I checked for you on irc but you weren't around. 2 other
> qualified people were and I wanted to get this out of my queue, so I pushed it.
> I don't think its the end of the world and patches can be backed out easily.

We're a multi-time-zone group, you won't always get feedback within a couple of hours.  And this wasn't a time-critical patch.

As for changing reviewers, I have been assuming that it is not the done thing, but I don't know -- is the policy clear on that?  https://developer.mozilla.org/en/Code_Review_FAQ doesn't address it.

> What do you want me to do with the patch? Put you up for review and wait until
> our ARM test coverage is back online?

First, you need to set up some way of doing ARM testing yourself, via scratchbox or whatever.

As for the patch, just re-push it.  It's not the patch itself that is a problem -- it's small -- so much as the breaking of protocols that concerned me:  testing by pushing to TM is clearly inappropriate, and changing reviewers when the first reviewer hasn't accepted the patch is possibly inappropriate.

Comment 10

[Andreas Gal :gal]

I have pushed-to-try into the TM tree before, and so have others. Maybe its not the best way of doing things. I have been considering TM a development/test/staging tree. Maybe its time to abandon that. Maybe others want to weigh in on that. I have rarely seen collisions due to this. We do conflict each other out at times (merge conflicts), but it was my impression so far that we didn't interfere with each other's landings on TM much. Maybe this impression is outdated.

As for the patch, I think its easiest if I invalidate/wontfix it. Its not mission critical as you pointed out, and a mere cleanup. I have a rc2 blocker I am supposed to look at. Again, sorry for the poor communication.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #10)
> 
> As for the patch, I think its easiest if I invalidate/wontfix it. Its not
> mission critical as you pointed out, and a mere cleanup. I have a rc2 blocker I
> am supposed to look at. Again, sorry for the poor communication.

I looked again at insCall() and I think it's ok.  So I'm happy for the patch to go in if it doesn't hurt soft_float, which I think requires some ARM testing?

We had process issues but the aim of the patch is good, let's not abandon it.  But it can wait for the rc2 blocker, of course.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/55a8910d8436

Comment 13

[Robert Sayre]

oops, I merged the checkin and then the backout. this is still open.

Comment 14

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to comment #14)
> These bugs are all part of a search I made for js bugs that are getting lost in
> transit:
> 
> http://tinyurl.com/jsDeadEndBugs
> 
> They all have a review+'ed, non-obsoleted patch and are not marked
> fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300
> days. Some of these got lost simply because the assignee/patch provider never
> requested a checkin, or just because they were forgotten about.

Thanks for doing this!  This bug is now obsolete, it got fixed in a different way:  LIR_callh (now called LIR_hcalli) is now only defined on softfloat platforms, none of which are 64-bits!