Bug 121056 – Add local to/from operand helpers similar to argument to/from operand

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

This is in preparation to invert the JSC stack.  Currently local virtual registers are positive.  When the stack is inverted they will be <= 0.  In many cases the code assumes that locals are positive.  A common pattern is to use an array index also as a virtual register operand.  Through the use of helpers that map local variable to/from operand these assumptions can be broken.  This should be patterns after the existing localToArgument helpers in Operands.h.

[Michael Saboff]

Created attachment 211119 [details]
Patch

[Geoffrey Garen]

Comment on attachment 211119 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:10
> +        registers in intended instead of the actual virtual register offset.  When the stack is

"in" should be "is".

"a index" should be "an index".

I think you mean "index into a data structure", or something like that. "index to the local registers" implies some kind of memory offset in a register bank, which the opposite of what you're doing.

> Source/JavaScriptCore/ChangeLog:18
> +        * bytecode/CodeBlock.cpp:
> +        (JSC::CodeBlock::registerName):
> +        * bytecode/CodeBlock.h:
> +        (JSC::CodeBlock::isCaptured):

Giant empty list not so helpful.

> Source/JavaScriptCore/dfg/DFGScoreBoard.h:92
> +            return (VirtualRegister)localToOperand(index);

I see this cast a lot. Why doesn't localToOperand return the VirtualRegister type?

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 211119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211119&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +        registers in intended instead of the actual virtual register offset.  When the stack is
> 
> "in" should be "is".
> 
> "a index" should be "an index".
> 
> I think you mean "index into a data structure", or something like that. "index to the local registers" implies some kind of memory offset in a register bank, which the opposite of what you're doing.
> 
> > Source/JavaScriptCore/ChangeLog:18
> > +        * bytecode/CodeBlock.cpp:
> > +        (JSC::CodeBlock::registerName):
> > +        * bytecode/CodeBlock.h:
> > +        (JSC::CodeBlock::isCaptured):
> 
> Giant empty list not so helpful.
> 
> > Source/JavaScriptCore/dfg/DFGScoreBoard.h:92
> > +            return (VirtualRegister)localToOperand(index);
> 
> I see this cast a lot. Why doesn't localToOperand return the VirtualRegister type?

I made the suggested changes.

[Michael Saboff]

Committed r155415: <http://trac.webkit.org/changeset/155415>

[Michael Saboff]

Comment on attachment 211119 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGScoreBoard.h:92
>> +            return (VirtualRegister)localToOperand(index);
> 
> I see this cast a lot. Why doesn't localToOperand return the VirtualRegister type?

It turns out that compilers don't treat the implicit enum to int correctly where a negative enum is used directly for an array index.  In that case it is treated as an unsigned.  I think it is better for us to cast an int result from localToOperand() to VirtualRegister than having to remember to cast an enum result to an (signed) int. The compiler flags an error when an int is used where an enum is needed, but silently does the wrong thing if an enum is used where an int is needed.  Besides, there are many other places where we use c-style casts to VirtualRegister.