Bug 212734 – Reduce DFGDoesGCCheck to only storing a uint32_t.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Paulo Matos]

Port DFGDoesGCCheck to 32 bits

[Paulo Matos]

Created attachment 401009 [details]
Patch

[Caio Lima]

Comment on attachment 401009 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:130
> +    static constexpr unsigned nodeIndexShift = 16;

Is 8 bits enough to encode nodeOp?

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:133
> +    static constexpr unsigned specialMask = 0x7fff;

Is this correct? This mask seems to be ok when we have 15 bits to store special, but here we only have 7.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:134
> +    static constexpr unsigned nodeOpMask = 0xffff;

The  same applies here.

[Caio Lima]

Comment on attachment 401009 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:66
> +#else

Wouldn't be possible to use `size_t` and avoid this divergence from 32-bits and 64-bits?

>> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:130
>> +    static constexpr unsigned nodeIndexShift = 16;
> 
> Is 8 bits enough to encode nodeOp?

For reference, `Node::op` into `dfg/DFGNode.h` uses 10 bits.

[Caio Lima]

I'm also checking that we don't have a proper setup of doesGC into `dfg/DFGSpeculativeJIT32_64.cpp` like we have for `dfg/DFGSpeculativeJIT64.cpp` into `SpeculativeJIT::compile(Node* node)`. It would be necessary to include this for proper 32-bits support.

[Paulo Matos]

Created attachment 401013 [details]
Patch

[Paulo Matos]

(In reply to Caio Lima from comment #3)
> Comment on attachment 401009 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401009&action=review
> 
> > Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:66
> > +#else
> 
> Wouldn't be possible to use `size_t` and avoid this divergence from 32-bits
> and 64-bits?
> 
> >> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:130
> >> +    static constexpr unsigned nodeIndexShift = 16;
> > 
> > Is 8 bits enough to encode nodeOp?
> 
> For reference, `Node::op` into `dfg/DFGNode.h` uses 10 bits.

Thanks for the comments - I have addressed this. Now nodeOp has 15 bits available.

[Paulo Matos]

(In reply to Caio Lima from comment #4)
> I'm also checking that we don't have a proper setup of doesGC into
> `dfg/DFGSpeculativeJIT32_64.cpp` like we have for
> `dfg/DFGSpeculativeJIT64.cpp` into `SpeculativeJIT::compile(Node* node)`. It
> would be necessary to include this for proper 32-bits support.

I hadn't noticed that - I will take a look.

[Paulo Matos]

Created attachment 401014 [details]
Patch

[Mark Lam]

Comment on attachment 401014 [details]
Patch

r- because this appears to be breaking the build (see mac-debug bot).  I also have an alternate solution which I'll try to upload later today.

[Paulo Matos]

Created attachment 401022 [details]
Patch

[Paulo Matos]

(In reply to Mark Lam from comment #9)
> Comment on attachment 401014 [details]
> Patch
> 
> r- because this appears to be breaking the build (see mac-debug bot).  I
> also have an alternate solution which I'll try to upload later today.

Yes, I forgot to define a variable for 64bit. Now added new patch which should fix it.

[Mark Lam]

Comment on attachment 401022 [details]
Patch

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

There is some value in this patch, but the 32-bit encoding is just wrong and won't work.  As I said before, I already have a patch brewing that will solve the 32-bit encoding problem.  Since the build is no longer breaking, can you wait for my patch?

> Source/JavaScriptCore/ChangeLog:10
> +        This change implements a port of the initial patch for 32bits. As it stands, 32bits ports
> +        are not compiling.

It's no longer true the 32-bits ports are not compiling.  Can you remove this?

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:50
> +    static size_t encode(bool expectDoesGC, unsigned nodeIndex, unsigned nodeOp)

size_t is the wrong type to use here.  This can break the build for the arm64_32 port.  I recommend that you add the following at the top of this struct:

#if USE(JSVALUE64)
using EncodedWord = uint64_t;
#else
using EncodedWord = uint32_t;
#endif

... and use EncodedWord here instead of size_t.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:67
> -
> +    

please undo this.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:114
> +    static constexpr unsigned specialShift = 1;
> +    static constexpr unsigned nodeOpShift = 1;

This is wrong.  Based on the present encoding, the special section will collide with the nodeOp section, and you will get non-sensical results from the doesGC error messages.

[Paulo Matos]

(In reply to Mark Lam from comment #12)
> Comment on attachment 401022 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401022&action=review
> 
> There is some value in this patch, but the 32-bit encoding is just wrong and
> won't work.  As I said before, I already have a patch brewing that will
> solve the 32-bit encoding problem.  Since the build is no longer breaking,
> can you wait for my patch?
> 

Thanks for the comments - I will wait for your patch as per your suggestion.

[Mark Lam]

Created attachment 401030 [details]
proposed patch.

[Mark Lam]

Created attachment 401037 [details]
proposed patch.

[Mark Lam]

Created attachment 401038 [details]
proposed patch.

[Mark Lam]

Created attachment 401042 [details]
proposed patch.

[Mark Lam]

Created attachment 401043 [details]
proposed patch.

[Caio Lima]

Comment on attachment 401043 [details]
proposed patch.

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

Informal r+ with comments. I tested compiling 32-bits locally (queue is not going to process it anytime soon) and both Release and Debug build if we  just add `#include "DFGDoesGC.h"` in `DFGSpeculativeJIT32_64.cpp`.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.cpp:45
> +    // We do this chck here just so we don't have to #include DFGNodeType.h

typo: s/chck/check/

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1810
> +        bool expectDoesGC = doesGC(m_jit.graph(), node);

We are missing `#include "DFGDoesGC.h"` into this file. I tested the patch locally and it is causing compilation error.

[Saam Barati]

Comment on attachment 401043 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:28
> +        This patch also makes ENABLE_DFG_DOES_GC_VALIDATION dependent on ENABLE(DFG_JIT).

can we also add a runtime flag for this to make it easy not to clutter the emitted assembly on debug builds?

[Mark Lam]

(In reply to Saam Barati from comment #20)
> Comment on attachment 401043 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401043&action=review
> 
> > Source/JavaScriptCore/ChangeLog:28
> > +        This patch also makes ENABLE_DFG_DOES_GC_VALIDATION dependent on ENABLE(DFG_JIT).
> 
> can we also add a runtime flag for this to make it easy not to clutter the
> emitted assembly on debug builds?

I'll do this in a separate patch.  Thanks.

[Mark Lam]

Thanks for the reviews.  Landing in r262562: <http://trac.webkit.org/r262562>.

[Radar WebKit Bug Importer]

<rdar://problem/63991469>