Bug 121056 - Add local to/from operand helpers similar to argument to/from operand
Summary: Add local to/from operand helpers similar to argument to/from operand
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 118758
  Show dependency treegraph
 
Reported: 2013-09-09 15:54 PDT by Michael Saboff
Modified: 2013-09-10 17:55 PDT (History)
0 users

See Also:


Attachments
Patch (59.80 KB, patch)
2013-09-09 16:55 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2013-09-09 15:54:21 PDT
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.
Comment 1 Michael Saboff 2013-09-09 16:55:19 PDT
Created attachment 211119 [details]
Patch
Comment 2 Geoffrey Garen 2013-09-09 17:22:02 PDT
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?
Comment 3 Michael Saboff 2013-09-09 22:36:04 PDT
(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.
Comment 4 Michael Saboff 2013-09-09 22:38:58 PDT
Committed r155415: <http://trac.webkit.org/changeset/155415>
Comment 5 Michael Saboff 2013-09-10 17:55:50 PDT
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.