Bug 127709 - FTL ArithMod case needs to be factored for hackability and correctness rather than just minimizing the amount of code
Summary: FTL ArithMod case needs to be factored for hackability and correctness rather...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
Depends on:
Blocks: 112840
  Show dependency treegraph
Reported: 2014-01-27 14:24 PST by Filip Pizlo
Modified: 2014-01-27 14:33 PST (History)
9 users (show)

See Also:

the patch (10.14 KB, patch)
2014-01-27 14:25 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-01-27 14:24:45 PST
The refactoring to make ArithDiv and ArithMod share code created two bugs.  One was fixed some time ago and I don't want to fix the other one.  The issue here is that although the two operations appear at first to have the same lowering, they actually have subtle differences.  Trying to make them share code will cause bugs.  Some of those bugs will be correctness bugs and then maybe you will catch them, but other times they will be performance bugs - like a 2x regression in audio-oscillator - and then maybe you'll overlook them.

The correct solution is to roll out the offending fix and to be more careful about changing ArithMod in the future.  It has historically been the case that everytime someone tries to refactor ArithMod lowering, they introduce bugs.  Sometimes those bugs are really hard to track down.  I'm really tired of having to fix ArithMod bugs.
Comment 1 Filip Pizlo 2014-01-27 14:25:51 PST
Created attachment 222357 [details]
the patch

Already rs=markH.
Comment 2 Filip Pizlo 2014-01-27 14:33:01 PST
Landed in http://trac.webkit.org/changeset/162868