Bug 122925

Summary: Transition remaining op_get* JITStubs to JIT operations
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 122287    
Attachments:
Description Flags
the patch. ggaren: review+

Mark Lam
Reported 2013-10-16 16:41:43 PDT
Transitioning: cti_op_get_by_id_generic cti_op_get_by_val cti_op_get_by_val_generic cti_op_get_by_val_string
Attachments
the patch. (26.97 KB, patch)
2013-10-16 23:40 PDT, Mark Lam 		ggaren: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2013-10-16 23:40:07 PDT
Created attachment 214422 [details] the patch. Turns out the failures I saw wile testing this change were due to https://bugs.webkit.org/show_bug.cgi?id=122938. With 122938 now fixed, this patch passes the javascriptcore tests.
Mark Lam
Comment 2 2013-10-16 23:46:20 PDT
Comment on attachment 214422 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=214422&action=review > Source/JavaScriptCore/jit/JITOperations.cpp:1344 > +EncodedJSValue JIT_OPERATION operationGetByValDefault(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedSubscript) I'm not sure that "operationGetByValDefault" is the best name for this function. I had to rename it because there's already a DFG operationGetByVal. Looking at the code, this function seems to be a more generic get_by_val then operationGetByValGeneric. As such, I was not able to tease out why the original JITStub author chose the naming scheme for these get_by_val inline cache implementations. Please suggest a better name if you have one. Thanks.
Geoffrey Garen
Comment 3 2013-10-17 00:13:23 PDT
Comment on attachment 214422 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=214422&action=review r=me >> Source/JavaScriptCore/jit/JITOperations.cpp:1344 >> +EncodedJSValue JIT_OPERATION operationGetByValDefault(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedSubscript) > > I'm not sure that "operationGetByValDefault" is the best name for this function. I had to rename it because there's already a DFG operationGetByVal. Looking at the code, this function seems to be a more generic get_by_val then operationGetByValGeneric. As such, I was not able to tease out why the original JITStub author chose the naming scheme for these get_by_val inline cache implementations. Please suggest a better name if you have one. Thanks. I think the naming here is fine, since this is the default function we call before any inline caching. I think the fundamental issue here is that we need to consolidate helper functions between the DFG, the baseline JIT, and the LLInt -- now that they all use the C calling convention.
Mark Lam
Comment 4 2013-10-17 00:19:53 PDT
Thanks for the review. Landed in r157559: <http://trac.webkit.org/r157559>.
Note You need to log in before you can comment on or make changes to this bug.