Bug 155164 – [JSC] Add register reuse for ArithAdd of an Int32 and constant in DFG

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Add register reuse for ArithAdd of an Int32 and constant in DFG

[Benjamin Poulain]

Created attachment 273280 [details]
Patch

[Benjamin Poulain]

Created attachment 273281 [details]
Patch

[Geoffrey Garen]

Comment on attachment 273281 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 273281 [details]
Patch

Clearing flags on attachment: 273281

Committed r197994: <http://trac.webkit.org/changeset/197994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Geoffrey Garen]

Let's roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155368

[Benjamin Poulain]

I'll look into that later, this is not a critical patch, just nice to have.

[Benjamin Poulain]

Reopening to attach new patch.

[Benjamin Poulain]

Created attachment 273786 [details]
Patch

[Benjamin Poulain]

That was trivial: branchAdd with immediate was missing blinding support.

[Benjamin Poulain]

                                    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

[Mark Lam]

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)".

[Mark Lam]

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.

[Benjamin Poulain]

Created attachment 276851 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 276851 [details]
Patch for landing

Clearing flags on attachment: 276851

Committed r199792: <http://trac.webkit.org/changeset/199792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.