Bytecode liveness analysis should have more lambdas and fewer sets

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 248552 [details]
the patch

[WebKit Commit Bot]

Attachment 248552 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:127:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:145:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:149:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 248552 [details]
the patch

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

r=me

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:130
> +    computeDefsForBytecodeOffset(
> +        codeBlock, bytecodeOffset,
> +        [&] (CodeBlock* codeBlock, Instruction*, OpcodeID, int operand) {
> +            if (isValidRegisterForLiveness(codeBlock, operand))
> +                def(indexForOperand(codeBlock, operand));
> +        });
> +    computeUsesForBytecodeOffset(
> +        codeBlock, bytecodeOffset,
> +        [&] (CodeBlock* codeBlock, Instruction*, OpcodeID, int operand) {
> +            if (isValidRegisterForLiveness(codeBlock, operand))
> +                use(indexForOperand(codeBlock, operand));
> +        });

nit: Everywhere else, we deal with the use case before the def case (including the original code, the template parameters. and the order the functions are defined in the caller).  Swapping these 2 calls will make it easier to read.

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 248552 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248552&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:130
> > +    computeDefsForBytecodeOffset(
> > +        codeBlock, bytecodeOffset,
> > +        [&] (CodeBlock* codeBlock, Instruction*, OpcodeID, int operand) {
> > +            if (isValidRegisterForLiveness(codeBlock, operand))
> > +                def(indexForOperand(codeBlock, operand));
> > +        });
> > +    computeUsesForBytecodeOffset(
> > +        codeBlock, bytecodeOffset,
> > +        [&] (CodeBlock* codeBlock, Instruction*, OpcodeID, int operand) {
> > +            if (isValidRegisterForLiveness(codeBlock, operand))
> > +                use(indexForOperand(codeBlock, operand));
> > +        });
> 
> nit: Everywhere else, we deal with the use case before the def case
> (including the original code, the template parameters. and the order the
> functions are defined in the caller).  Swapping these 2 calls will make it
> easier to read.

It's true that we do that everywhere else; whenever the ordering doesn't matter we say "use" before we say "def".  But here the ordering does matter.  We must process defs before we process uses.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/181467