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...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 112840
  Show dependency tree / graph
 
Reported: 2014-01-27 14:24 PST by Filip Pizlo
Modified: 2014-01-27 14:33 PST (History)
9 users (show)

See Also:

Attachments
the patch (10.14 KB, patch)
2014-01-27 14:25 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

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