Bug 113146 – Offlineasm cloop backend compiles op+branch incorrectly

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

This is almost certainly a benign bug but it would be worth fixing. 

Quoth Julien:

sc.rb unless there is a strong reason not to.
> I started to look at risc.rb and there are some things that I don't understand.
> 
> For instance the riscLowerSimpleBranchOps, where we have something like this:
>   # baddiz foo, bar, baz
>   #
>   # will become:
>   #
>   # addi foo, bar
>   # bz baz
> 
> As I see in the C loop impl, I thought it would be something like:
>   # baddiz foo, bar, baz
>   #
>   # will become:
>   #
>   # addi foo, bar, tmp
>   # bz baz
>   # move tmp, bar
> 
> Am I missing something here ?

I confirmed by inspection that this is indeed what cloop does, in which case it's wrong. I expect this to be asymptommatic for now since if the LLInt uses those ops, it will be branching to a place where 'bar' is dead.

[Mark Lam]

I asked and Filip answered:

Me: I take it that the bug reported here is that the result should be applied before the branch, and is not conditional on the branch.
Filip: Yup. 
Me:   I presume that this should be true of the following opcodes also? baddio, bsubio, bmulio, baddis, baddinz, baddqs, baddqz, baddqnz, baddps, baddpz, baddpnz, bsubis, bsubiz, bsubinz, borris, borriz, borrinz.
Filip: Yup.

[Mark Lam]

Created attachment 194958 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 194958 [details]
the patch.

r=me

Is there a way to regression test for this, when the c loop happens to be enabled?

[Mark Lam]

(In reply to comment #3)
> Is there a way to regression test for this, when the c loop happens to be enabled?

I'll think about it and file a bug to do this later if it's possible (with reasonable effort).

Thanks for the review.  Landed in r146831: <http://trac.webkit.org/changeset/146831>.

[Mark Lam]

(In reply to comment #4)
> (In reply to comment #3)
> > Is there a way to regression test for this, when the c loop happens to be enabled?
> 
> I'll think about it and file a bug to do this later if it's possible (with reasonable effort).

After inspecting the llint asm code, I don't think there is any JS level test that can check to ensure that the operations is done before the branch checks (at least at this time).  This is because the only existing uses of these instructions are "baddis" which is only when JIT_ENABLED (hence, not applicable to the cloop), and the branch on integer overflow cases.  In the integer overflow cases, the overflow condition will result in a branch to slow code which redo the operation using doubles.  Hence, we won't be able to observe that the instruction did not store the result of the integer operation.