Minor dominator-tree computation cleanups.

RESOLVED FIXED in mozilla24

Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sunfish, Unassigned)

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

6 years ago
A few things in the dominator-tree computation code could be slightly simplified.
Reporter

Comment 1

6 years ago
Attachment #760525 - Flags: review?(sstangl)
Attachment #760525 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/ca991d0bb5ea
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reading the summary, I thought this is only a code change and not a behaviour change. So this shouldn't affect performance at all, right?

I think this introduced a regression on awfy-assorted misc-basic-intops of 6.7%. This or 
bug 881366. But that bug looks like it should only improve performance. Here I'm not certain this is only a esthetic change. I think it also behaviour change.
Reporter

Comment 5

6 years ago
Yes, this patch shouldn't affect performance at all. I examined the IONFLAGS=all output on misc-basic-intops.js with and without this patch and they are identical, ignoring pointer differences. If there's anything else you'd like me to examine, please suggest it.

My best guess for the performance regression on misc-basic-intops is an innocent alignment change due to the patch in bug 881366. The generated code for the mul loop is:

 [Codegen] movabsq    $0x2606d08, %r11
 [Codegen] cmpl       $0x0, 0x0(%r11d)
 [Codegen] jne        ((380))
 [Codegen] cmpl       %ecx, %edx
 [Codegen] jge        ((388))
 [Codegen] movq       %rax, %rbp
 [Codegen] imull      %ebx, %ebp
 [Codegen] jo         ((400))
 [Codegen] testl      %ebp, %ebp
 [Codegen] je         ((408))
 [Codegen] addl       $0x1, %edx
 [Codegen] movq       %rbp, %rax
 [Codegen] jmp        ((419))

The add loop is similar. It's the same with or without either the patch in this bug or the patch in bug 881366. Also note that Ion does not currently align loops.

There are numerous opportunities for improvement here, but I don't see anything related to these patches. If you know of anything relevant that I can investigate here, please let me know.
After testing more thoroughly, this must indeed be one of those aligning issues, that show a regression that isn't caused by the code changes. Thanks for looking into it!
Resolution: FIXED → INVALID
Uhm oops, I thought I was busy in a separate bug. Sorry!
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.