Summary: | Add local to/from operand helpers similar to argument to/from operand | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 118758 | ||||||
Attachments: |
|
Description
Michael Saboff
2013-09-09 15:54:21 PDT
Created attachment 211119 [details]
Patch
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? (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. Committed r155415: <http://trac.webkit.org/changeset/155415> 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. |