Bug 155164

Summary: [JSC] Add register reuse for ArithAdd of an Int32 and constant in DFG
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, keith_miller, mark.lam, msaboff, ossy, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155355, 155368    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Benjamin Poulain 2016-03-08 00:25:48 PST
[JSC] Add register reuse for ArithAdd of an Int32 and constant in DFG
Comment 1 Benjamin Poulain 2016-03-08 00:27:53 PST
Created attachment 273280 [details]
Patch
Comment 2 Benjamin Poulain 2016-03-08 00:55:56 PST
Created attachment 273281 [details]
Patch
Comment 3 Geoffrey Garen 2016-03-10 20:54:04 PST
Comment on attachment 273281 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2016-03-10 22:04:38 PST
Comment on attachment 273281 [details]
Patch

Clearing flags on attachment: 273281

Committed r197994: <http://trac.webkit.org/changeset/197994>
Comment 5 WebKit Commit Bot 2016-03-10 22:04:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2016-03-11 02:44:03 PST
(In reply to comment #4)
> Comment on attachment 273281 [details]
> Patch
> 
> Clearing flags on attachment: 273281
> 
> Committed r197994: <http://trac.webkit.org/changeset/197994>

It made 2 JSC stress tests crash (at least) on ARM 32-bit and 64-bit Linux bots.
See https://bugs.webkit.org/show_bug.cgi?id=155355 for details.
Comment 7 Geoffrey Garen 2016-03-11 09:09:16 PST
Let's roll out.
Comment 8 WebKit Commit Bot 2016-03-11 09:47:23 PST
Re-opened since this is blocked by bug 155368
Comment 9 Benjamin Poulain 2016-03-11 14:04:14 PST
I'll look into that later, this is not a critical patch, just nice to have.
Comment 10 Benjamin Poulain 2016-03-11 16:53:26 PST
Reopening to attach new patch.
Comment 11 Benjamin Poulain 2016-03-11 16:53:27 PST
Created attachment 273786 [details]
Patch
Comment 12 Benjamin Poulain 2016-03-11 16:54:11 PST
That was trivial: branchAdd with immediate was missing blinding support.
Comment 13 Benjamin Poulain 2016-04-20 01:26:44 PDT
                                    Conf#1                    Conf#2                                      

3d-cube                         5.0156+-0.1633            4.8687+-0.1295          might be 1.0302x faster
3d-morph                        5.0690+-0.0673            4.9932+-0.0474          might be 1.0152x faster
3d-raytrace                     5.4584+-0.3212            5.2790+-0.1017          might be 1.0340x faster
access-binary-trees             2.0642+-0.0457     ?      2.1047+-0.0746        ? might be 1.0196x slower
access-fannkuch                 5.9108+-0.1256            5.9075+-0.0320        
access-nbody                    2.5262+-0.0326     ?      2.5738+-0.0687        ? might be 1.0188x slower
access-nsieve                   3.0632+-0.1334            2.9922+-0.0496          might be 1.0237x faster
bitops-3bit-bits-in-byte        1.1284+-0.0188     ?      1.1367+-0.0268        ?
bitops-bits-in-byte             2.9306+-0.1908            2.8443+-0.0557          might be 1.0303x faster
bitops-bitwise-and              2.0470+-0.0190     ?      2.0562+-0.0361        ?
bitops-nsieve-bits              3.1448+-0.0657            3.1123+-0.0526          might be 1.0104x faster
controlflow-recursive           2.4241+-0.1033            2.3214+-0.0180          might be 1.0442x faster
crypto-aes                      4.1147+-0.0601            4.0947+-0.0811        
crypto-md5                      2.4777+-0.0579     ?      2.4813+-0.0644        ?
crypto-sha1                     2.3146+-0.0534            2.2861+-0.0220          might be 1.0124x faster
date-format-tofte               6.5533+-0.1548     ?      6.8088+-0.2330        ? might be 1.0390x slower
date-format-xparb               4.6342+-0.1291            4.5318+-0.0513          might be 1.0226x faster
math-cordic                     2.8395+-0.0559     ?      2.8521+-0.0414        ?
math-partial-sums               4.8627+-0.0715     ?      4.8931+-0.1341        ?
math-spectral-norm              1.9981+-0.0322     ?      2.0022+-0.0301        ?
regexp-dna                      6.4825+-0.1434     ?      6.5287+-0.1690        ?
string-base64                   4.8413+-0.1875            4.8092+-0.1618        
string-fasta                    5.7983+-0.0716            5.7192+-0.0556          might be 1.0138x faster
string-tagcloud                 8.1424+-0.1675            8.1003+-0.1894        
string-unpack-code             19.3036+-0.6727           18.8612+-0.2572          might be 1.0235x faster
string-validate-input           4.4333+-0.1465            4.2969+-0.0581          might be 1.0317x faster

<arithmetic>                    4.5992+-0.0331            4.5560+-0.0154          might be 1.0095x faster
Comment 14 Mark Lam 2016-04-20 10:02:04 PDT
Comment on attachment 273786 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssembler.h:1710
> +            // If we don't have a scratch register available for use, we'll just
> +            // place a random number of nops.
> +            uint32_t nopCount = random() & 3;
> +            while (nopCount--)
> +                nop();

Is this really adequate as a replacement for constant blinding?  Seems like this gives an attacker a 1 in 3 chance of getting what he wants.

> Source/JavaScriptCore/dfg/DFGOSRExit.h:68
>      SpeculationRecovery(SpeculationRecoveryType type, GPRReg dest, GPRReg src)
> -        : m_type(type)
> +        : m_src(src)
>          , m_dest(dest)
> -        , m_src(src)
> +        , m_type(type)
> +    {
> +    }

Add an "ASSERT(type != SpeculativeAddImmediate)" here?

> Source/JavaScriptCore/dfg/DFGOSRExit.h:75
> +    SpeculationRecovery(SpeculationRecoveryType type, GPRReg dest, int32_t immediate)
> +        : m_immediate(immediate)
> +        , m_dest(dest)
> +        , m_type(type)
>      {
>      }

Either add an "ASSERT(type == SpeculativeAddImmediate)" here, or just remove the option to pass in a type, and hardwire "m_type(SpeculativeAddImmediate)".
Comment 15 Mark Lam 2016-04-20 10:10:43 PDT
Comment on attachment 273786 [details]
Patch

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

r=me.  Please consider suggestions for SpeculationRecovery before landing.

>> Source/JavaScriptCore/assembler/MacroAssembler.h:1710
>> +                nop();
> 
> Is this really adequate as a replacement for constant blinding?  Seems like this gives an attacker a 1 in 3 chance of getting what he wants.

Nevermind.  Seems like this is already accepted standard practice in the code base.
Comment 16 Benjamin Poulain 2016-04-20 14:25:27 PDT
Created attachment 276851 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2016-04-20 15:24:15 PDT
Comment on attachment 276851 [details]
Patch for landing

Clearing flags on attachment: 276851

Committed r199792: <http://trac.webkit.org/changeset/199792>
Comment 18 WebKit Commit Bot 2016-04-20 15:24:21 PDT
All reviewed patches have been landed.  Closing bug.