...
Created attachment 415330 [details] patch
Comment on attachment 415330 [details] patch r=me
Comment on attachment 415330 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=415330&action=review Looks like this code was always broken for 32-bit. Can you pass a JSValueRegs to emitValueProfilingSite(), and at least add a FIXME for the 32-bit folks to fix JIT::emit_op_iterator_next()? > Source/JavaScriptCore/jit/JITInlines.h:312 > +inline void JIT::emitValueProfilingSite(ValueProfile& valueProfile, GPRReg value) Use JSValueRegs instead of GPRReg. > Source/JavaScriptCore/jit/JITInlines.h:317 > const RegisterID valueTag = regT1; This is going to be a bug because doneGPR above is regT1. I think the right thing to do is to pass value using the JSValueRegs abstraction instead.
(In reply to Mark Lam from comment #3) > Comment on attachment 415330 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415330&action=review > > Looks like this code was always broken for 32-bit. Can you pass a > JSValueRegs to emitValueProfilingSite(), and at least add a FIXME for the > 32-bit folks to fix JIT::emit_op_iterator_next()? > > > Source/JavaScriptCore/jit/JITInlines.h:312 > > +inline void JIT::emitValueProfilingSite(ValueProfile& valueProfile, GPRReg value) > > Use JSValueRegs instead of GPRReg. > > > Source/JavaScriptCore/jit/JITInlines.h:317 > > const RegisterID valueTag = regT1; > > This is going to be a bug because doneGPR above is regT1. I think the right > thing to do is to pass value using the JSValueRegs abstraction instead. I don't care about 32-bits
(In reply to Saam Barati from comment #4) > (In reply to Mark Lam from comment #3) > > Comment on attachment 415330 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=415330&action=review > > > > Looks like this code was always broken for 32-bit. Can you pass a > > JSValueRegs to emitValueProfilingSite(), and at least add a FIXME for the > > 32-bit folks to fix JIT::emit_op_iterator_next()? > > > > > Source/JavaScriptCore/jit/JITInlines.h:312 > > > +inline void JIT::emitValueProfilingSite(ValueProfile& valueProfile, GPRReg value) > > > > Use JSValueRegs instead of GPRReg. > > > > > Source/JavaScriptCore/jit/JITInlines.h:317 > > > const RegisterID valueTag = regT1; > > > > This is going to be a bug because doneGPR above is regT1. I think the right > > thing to do is to pass value using the JSValueRegs abstraction instead. > > I don't care about 32-bits I suppose we can use the abstraction anyways
Created attachment 415348 [details] patch for landing
Comment on attachment 415330 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=415330&action=review >> Source/JavaScriptCore/jit/JITInlines.h:317 >> const RegisterID valueTag = regT1; > > This is going to be a bug because doneGPR above is regT1. I think the right thing to do is to pass value using the JSValueRegs abstraction instead. to clarify, this wouldn't have been a bug because the other code you're reading is 64-bit only. However, JSValueRegs is a nicer abstraction, so I went with it as you suggest
Committed r270423: <https://trac.webkit.org/changeset/270423> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415348 [details].
<rdar://problem/71964541>
Comment on attachment 415348 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=415348&action=review > Source/JavaScriptCore/jit/JIT.h:377 > + void emitValueProfilingSite(ValueProfile&, JSValueRegs value = JSValueRegs { regT0 }); I think it would be better to require an explicit JSValueRegs to avoid future mistakes. I also found the same issue on `JIT::emit_op_get_prototype_of`. If you agree, I can include such change on https://bugs.webkit.org/show_bug.cgi?id=219535.
(In reply to Caio Lima from comment #10) > Comment on attachment 415348 [details] > patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415348&action=review > > > Source/JavaScriptCore/jit/JIT.h:377 > > + void emitValueProfilingSite(ValueProfile&, JSValueRegs value = JSValueRegs { regT0 }); > > I think it would be better to require an explicit JSValueRegs to avoid > future mistakes. I also found the same issue on > `JIT::emit_op_get_prototype_of`. If you agree, I can include such change on > https://bugs.webkit.org/show_bug.cgi?id=219535. I agree.
(In reply to Saam Barati from comment #11) > (In reply to Caio Lima from comment #10) > > Comment on attachment 415348 [details] > > patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=415348&action=review > > > > > Source/JavaScriptCore/jit/JIT.h:377 > > > + void emitValueProfilingSite(ValueProfile&, JSValueRegs value = JSValueRegs { regT0 }); > > > > I think it would be better to require an explicit JSValueRegs to avoid > > future mistakes. I also found the same issue on > > `JIT::emit_op_get_prototype_of`. If you agree, I can include such change on > > https://bugs.webkit.org/show_bug.cgi?id=219535. > > I agree. Since https://bugs.webkit.org/show_bug.cgi?id=219535 already landed, I created https://bugs.webkit.org/show_bug.cgi?id=219550 to handle it.