Bug 122170 – FTL: split overflow checks into non-overflow arithmetic and an additional call to the overflow intrinsic check.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nadav Rotem]

FTL: split overflow checks into non-overflow arithmetic and an additional call to the overflow intrinsic check.

[Nadav Rotem]

Created attachment 213113 [details]
Patch

[Geoffrey Garen]

Comment on attachment 213113 [details]
Patch

r=me

Does SelectionDAG also catch the multiply case?

[Geoffrey Garen]

Comment on attachment 213113 [details]
Patch

Sorry, one style nit: The "Reviewed by" line needs to be right under the bug name/URL pair. I think some of our automated tools expect to find it there.

[Nadav Rotem]

Okay. Should I resubmit ?

[Nadav Rotem]

Yes. It works with Mul, Add and Inc.   I am sure that we are missing some patterns but we can fix them as we go. 

This:

function foo() {
  var sum = 1
  while (sum < 100000000)
    sum *= 10
}

Becomes this:

LBB0_1:                                       ## =>This Inner Loop Header: Depth=1
	imull	$10, %eax, %ecx
	jo	LBB0_7

## BB#2:                                ## %OSR exit continuation for @11<Int32>
                                              ##   in Loop: Header=BB0_1 Depth=1
	testl	%ecx, %ecx
	je	LBB0_3
LBB0_4:                                 ## %ArithMul continuation
                                             ##   in Loop: Header=BB0_1 Depth=1
	cmpl	$100000000, %ecx        ## imm = 0x5F5E100
	movl	%ecx, %eax
	jl	LBB0_1
	jmp	LBB0_5

[Geoffrey Garen]

> Okay. Should I resubmit ?

Yes please.

[Nadav Rotem]

Created attachment 213117 [details]
Patch

[Nadav Rotem]

Added commit-queue=?.

[Filip Pizlo]

Comment on attachment 213117 [details]
Patch

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

Looks great!

> Source/JavaScriptCore/ChangeLog:36
> +LBB0_1:                                 ## %OSR exit continuation for @24<Int32>
> +                                        ## =>This Inner Loop Header: Depth=1
> +    movl  %ecx, %esi
> +    incl  %esi
> +    jo  LBB0_4

I would indent this.

[Nadav Rotem]

Indent the commit message ? In what way ?

[Filip Pizlo]

(In reply to comment #10)
> Indent the commit message ? In what way ?

The ChangeLog messages should have at least eight-space indent except for the header ("2013-10-01  Nadav Rotem  <[e-mail]>").  So this part of your patch:

+LBB0_1:                                 ## %OSR exit continuation for @24<Int32>
+                                        ## =>This Inner Loop Header: Depth=1
+    movl  %ecx, %esi
+    incl  %esi
+    jo  LBB0_4

Should be:

+        LBB0_1:                                 ## %OSR exit continuation for @24<Int32>
+                                                ## =>This Inner Loop Header: Depth=1
+            movl  %ecx, %esi
+            incl  %esi
+            jo  LBB0_4

We have some scripts that try to parse changelog's, so sticking to the format is usually a good idea.

[Nadav Rotem]

Created attachment 213124 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 213124 [details]
Patch

Clearing flags on attachment: 213124

Committed r156746: <http://trac.webkit.org/changeset/156746>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.