Bug 226817 – [JSC] Fix incorrect register reuse in 32bit after r278568

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Xan Lopez]

The JSVALUE32_64 branch potentially needs both the tag and payload registers for both left/right nodes, so we cannot reuse any of them for the result since the first thing the code does is set it zero. Just remove the Reuse construction. This was causing extremely long test runs on 32bit.

[Xan Lopez]

Created attachment 430963 [details]
Fix incorrect register reuse

v1

[Angelos Oikonomopoulos]

*** Bug 226815 has been marked as a duplicate of this bug. ***

[Caio Lima]

Comment on attachment 430963 [details]
Fix incorrect register reuse

View in context: https://bugs.webkit.org/attachment.cgi?id=430963&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7388
> +    GPRTemporary result(this);

Could we try to still reuse it, but then move 0 after `notEqual.link(&m_jit);` on line 7404? I think this would allow us to reuse left and right payload/tag there.

[Caio Lima]

Comment on attachment 430963 [details]
Fix incorrect register reuse

View in context: https://bugs.webkit.org/attachment.cgi?id=430963&action=review

r=me

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7388
>> +    GPRTemporary result(this);
> 
> Could we try to still reuse it, but then move 0 after `notEqual.link(&m_jit);` on line 7404? I think this would allow us to reuse left and right payload/tag there.

Never mind, this would require another jump for Equals case. Not reusing the tag sounds a better compromise then generating multiple branchs

[Caio Lima]

Comment on attachment 430963 [details]
Fix incorrect register reuse

32-bits queue is quite long, committing this to make it process patches

[Caio Lima]

Comment on attachment 430963 [details]
Fix incorrect register reuse

I retract this change. I think it wont work because we need to set `false` to result when left.tagGPR(), right.tagGPR() are different.

[Caio Lima]

(In reply to Caio Lima from comment #6)
> Comment on attachment 430963 [details]
> Fix incorrect register reuse
> 
> I retract this change. I think it wont work because we need to set `false`
> to result when left.tagGPR(), right.tagGPR() are different.

The patch already does that, since it doesn’t change previous behavior. So, my previous comment is wrong. I’ll r+/cq+ again after local run shows it passes all tests.

[Caio Lima]

Comment on attachment 430963 [details]
Fix incorrect register reuse

r=me

[EWS]

Committed r278662 (238644@main): <https://commits.webkit.org/238644@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430963 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79081335>