Bug 138671 - Reduce amount of cut-and-paste needed for probe mechanism implementations
Summary: Reduce amount of cut-and-paste needed for probe mechanism implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 138681 138708
Blocks: 138660
  Show dependency treegraph
 
Reported: 2014-11-12 14:28 PST by Mark Lam
Modified: 2014-11-14 12:28 PST (History)
3 users (show)

See Also:


Attachments
the patch. (44.11 KB, patch)
2014-11-12 14:40 PST, Mark Lam
no flags Details | Formatted Diff | Diff
alternate implementation based on Geoff's suggestions (42.29 KB, patch)
2014-11-13 19:33 PST, Mark Lam
no flags Details | Formatted Diff | Diff
alternate implementation based on Geoff's suggestions + a missing ChangeLog comment. (42.49 KB, patch)
2014-11-13 19:43 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-11-12 14:28:16 PST
The present implementation requires that each MacroAssembler implementation provide their own copy of all of the probe implementations even when most of it is identical.  This patch reimplements the common parts into MacroAssemblerProbe.h and have each target specific MacroAssembler implementation inherit from it instead.  The target specific MacroAssembler only needs to implement a few target specific methods that are expected by and documented in MacroAssemblerProbe.h.  MacroAssembler ports that do not care to implement the probe mechanism simply need no inherit from MacroAssemblerProbe.

In this patch, I also fill out some missing xmm registers in the X86_64's FP register list.  Also simplified the X86 and X86_64 ports to use the same port implementation.

This is all done in preparation to add printing functionality in JITted code for debugging.
Comment 1 Mark Lam 2014-11-12 14:40:02 PST
Created attachment 241444 [details]
the patch.
Comment 2 WebKit Commit Bot 2014-11-12 14:42:12 PST
Attachment 241444 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:131:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:143:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerProbe.h:39:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerProbe.h:42:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:102:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:104:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:110:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:112:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:118:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:120:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:40:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:45:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:52:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:54:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:58:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:61:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:63:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:39:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:45:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:47:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:53:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:55:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/ARMAssembler.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 25 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Geoffrey Garen 2014-11-12 16:06:25 PST
Comment on attachment 241444 [details]
the patch.

Let's not build up a side definition of the CPU in the probing machinery -- instead, let's use the shared definition provided by the assembler.

(1) The assembler header should define the canonical list of registers in a macro.

(2) The definition of the register namespaces should use the canonical list.

(3) The probe should use the canonical list.

(4) The probe should treat FP registers as 64bit values, as the assembler does. This eliminates some abstractions like GPRegValue and FPRegValue. 

(5) If there's some need for a global definition of an "Assembler" type, that should bear the name "Assembler", rather than being specialized with a probe-related name. However, I'll note that the MacroAssembler class defines its assembler type as a dependent type, so there's probably no need for a global type name.

(6) Let's call CPUInfo CPUState, since that's a little clearer about recording the temporary state of the CPU, and let's put it in its own header, and remove the redeclaration of RegisterID and FPRegisterID.
Comment 4 Mark Lam 2014-11-13 19:33:05 PST
Created attachment 241531 [details]
alternate implementation based on Geoff's suggestions
Comment 5 Mark Lam 2014-11-13 19:43:12 PST
Created attachment 241534 [details]
alternate implementation based on Geoff's suggestions + a missing ChangeLog comment.
Comment 6 WebKit Commit Bot 2014-11-13 19:46:05 PST
Attachment 241534 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:843:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:846:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:860:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:873:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:885:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:891:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:40:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:45:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:52:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:54:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:60:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 11 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Geoffrey Garen 2014-11-14 10:45:50 PST
Comment on attachment 241534 [details]
alternate implementation based on Geoff's suggestions + a missing ChangeLog comment.

r=me
Comment 8 Mark Lam 2014-11-14 12:26:54 PST
Thanks for the review.  Landed in r176134: <http://trac.webkit.org/r176134>.