We should insert LIR_regfence before npe, interrupt, rangeCheck

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

unspecified
flash10.1
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
Created attachment 422397 [details] [diff] [review]
Patch

This will greatly reduce the likelihood that there will be (pointless) spill/restores around the branch sites.
(Assignee)

Updated

8 years ago
Attachment #422397 - Attachment is patch: true
Attachment #422397 - Attachment mime type: application/octet-stream → text/plain
Attachment #422397 - Flags: review?(edwsmith)

Comment 1

8 years ago
As a separate patch we should look at whether a regfence would be beneficial for the cold paths of the stack overflow check (which does return) or the setjmp branch to the exception handler dispatcher.

Comment 2

8 years ago
Comment on attachment 422397 [details] [diff] [review]
Patch

A tweak like this deserves some comments explaining the win, or a comment referencing this bug, with some evidence of the win.  but it looks fine;  nice tweak, too.
Attachment #422397 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 3

8 years ago
Good point, I'll insert examples & commentary from our offlist discussion in comments here before landing.
(Assignee)

Comment 4

8 years ago
This helps with situations like so... prior to patch, you could have code like

   ld61 = ld and2[992]
00133c5c13   mov ecx,992(esi)
00133c5c19   mov -108(ebp),ecx  <= spill ld61
                     RR eax(ld60) ecx(ld61) ebx(env) esi(and2)
   jt eq27 -> npe
00133c5c1c   test ecx,ecx
00133c5c1e   je    0x133c5d60
## merging registers (union) with existing edge
00133c5c24   mov ebx,-108(ebp)  <= restore ld61
                         RR eax(ld60) ebx(ld61) esi(and2) 

[npe]
0x133c5d60   opcode
                         RR ebx(whatever) 


i.e., since [npe] assumed contents of ebx, the branch to it had to do spill/restore.
(Assignee)

Comment 5

8 years ago
Edwin also comments (from an offline discussion): "LIR_regfence was added specifically for this case; it causes register assignments in a cold branch (like npe, or like a side exit, or an uncommon path for an inlined operator) to not propagate upwards past the regfence where they would add register pressure to the hot path."
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/tamarin-redux/rev/9d3819da173d

Updated

8 years ago
Assignee: nobody → stejohns

Updated

8 years ago
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
Target Milestone: --- → flash10.1

Updated

8 years ago
Priority: -- → P2

Comment 7

8 years ago
Landed a while ago.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 8

8 years ago
verified in argo: 9d3819da173d
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.