809485 - IonMonkey: don't stop at resumepoints in edgecase analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

In edgecase analysis we know if a value is only consumed as truncated integer. In that case it doesn't make sense to have bailout checks for that function. Now we stop when the result is used in a resumepoint, because we want after bailing to have the right values.

Now we can actually remove this check, because the value will indeed be incorrect, but there is no observable difference in output with this value or the truncated value. Also when we run in the interpreter.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Hannes Verschore [:h4writer]]

gkw, could you please use your fuzzer on this patch? I want to be sure I don't introduce any bugs! All tests succeeded for me, but it doesn't give me enough comfort to just push this. There could be some special internals preventing this.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 679190 [details] [diff] [review]
Patch

Setting some review/feedback flags to get them on the fuzzing radar (I wasn't able to find a second feedback? flag to set though).

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Hannes, is it possible for you to rebase it to tip? I tried applying it to 1437578c371f (tip at time of writing) but I seemed to get a patch failed error.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Patch

Really sorry. But somehow I added a patch for another bug. This should work. Thanks

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 679724 [details] [diff] [review]
Patch

+'ing because I did not hit anything immediately wrong after a few hours' of fuzzing.

Comment 7

[Christian Holler (:decoder)]

Comment on attachment 679724 [details] [diff] [review]
Patch

No issues found so far, feedback+ :)

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 679724 [details] [diff] [review]
Patch

We discussed this on IRC. The value could leak into the interpreter, but in normal execution the truncated value won't be visible and the computations are still correct. We win 1% on SS and would also win in v8-crypto (to hard to determine how much). On ARM it would be even bigger, because we lose the bailouts on MUL (mjrosenb told me it was really a pain on ARM).

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Christian Holler (:decoder) from comment #7)
> No issues found so far, feedback+ :)

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #6)
> +'ing because I did not hit anything immediately wrong after a few hours' of
> fuzzing.

Also thanks both!

Comment 10

[Hannes Verschore [:h4writer]]

I noticed I didn't put any numbers in here

Not patched: 
  Crypto: 8349

Patched: 
  Crypto: 8746

So an 5% increase on Crypto

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 679724 [details] [diff] [review]
Patch

So if I understand, let's say you have some operation like:

  var z = (x + y);
  ... 
  var t = (x + y) | 0;
  ... 

We could compile this as:
  
  1: add(x, y)
  2: resumepoint z=1
  3: add(x, y)
  4: or(3, #0)
  5: resumepoint t=4

GVN+EdgecaseAnalysis could fold this as:

  1: add(x, y)
  2: resumepoint z=1
  3: resumepoint t=1

Now, |z| *could* be the wrong value, but it doesn't actually matter since if anything had observed it, the edgecase analysis would have failed. Sounds right to me, as long as phis as treated as observable uses.

Comment 12

[Hannes Verschore [:h4writer]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7ebfa541fb86

Comment 13

[Hannes Verschore [:h4writer]]

(In reply to David Anderson [:dvander] from comment #11)
> ...

That is correct.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ebfa541fb86