Port DFGDoesGCCheck to 32 bits
Created attachment 401009 [details] Patch
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.
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.
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.
Created attachment 401013 [details] Patch
(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.
(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.
Created attachment 401014 [details] Patch
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.
Created attachment 401022 [details] Patch
(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.
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.
(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.
Created attachment 401030 [details] proposed patch.
Created attachment 401037 [details] proposed patch.
Created attachment 401038 [details] proposed patch.
Created attachment 401042 [details] proposed patch.
Created attachment 401043 [details] proposed patch.
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.
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?
(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.
Thanks for the reviews. Landing in r262562: <http://trac.webkit.org/r262562>.
<rdar://problem/63991469>