Bug 127709

Summary: FTL ArithMod case needs to be factored for hackability and correctness rather than just minimizing the amount of code
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 112840    
Attachments:
Description Flags
the patch none

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