881397 - Minor dominator-tree computation cleanups.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

A few things in the dominator-tree computation code could be slightly simplified.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

Comment 2

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca991d0bb5ea

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ca991d0bb5ea

Comment 4

[Hannes Verschore [:h4writer]]

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.

Comment 5

[Dan Gohman [:sunfish]]

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.

Comment 6

[Hannes Verschore [:h4writer]]

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!

Comment 7

[Hannes Verschore [:h4writer]]

Uhm oops, I thought I was busy in a separate bug. Sorry!