[Webkit-unassigned] [Bug 99706] MIPS LLInt implementation.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 5 23:18:43 PST 2013


https://bugs.webkit.org/show_bug.cgi?id=99706


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #181313|review?                     |review+
               Flag|                            |




--- Comment #51 from Filip Pizlo <fpizlo at apple.com>  2013-01-05 23:20:39 PST ---
(From update of attachment 181313)
View in context: https://bugs.webkit.org/attachment.cgi?id=181313&action=review

r=me, but either rename common/specific to something better or come up with a way of avoiding refactorings in risc.rb altogether.

> Source/JavaScriptCore/offlineasm/mips.rb:273
> +                comp = node.opcode[1,1] == "b" ? "sltub" : "sltu"

I tend to like consistency; having one style that we stick to makes it easier to understand the code at a glance.

So, I think it's valuable to stick to our space-after-comma convention throughout webkit.  So I would write node.opcode[1, 1].

But I would like it even better if for this purpose we used node.opcode[1..1], which makes it more obvious what is going on.

But what I would have liked even better is if you had simply said node.opcode[1] == ?b, which I believe is the more canonical Ruby way of testing the second character.

> Source/JavaScriptCore/offlineasm/mips.rb:379
> +                    tmp = SpecialRegister.new("$ra")

Any reason why you're not creating one singleton instance of SpecialRegister for each special register you need ($ra, $t9, etc)?  That's what the other backends do, and it preserves the invariant that identity equality implies that it's the same register.

>> Source/JavaScriptCore/offlineasm/mips.rb:549
>> +        result = mipsLowerMisplacedAddressesSpecific(result)
> 
> I see that you split riscLowerMisplacedAddresses() into riscLowerMisplacedAddressesCommon() and riscLowerMisplacedAddressesSpecific().  It seems quite arbitrary which instruction is considered common and which is platform specific.  I suggest you do the following instead:
> 
> 1. Undo the common and specific split in risk.rb.
> 2. Refactor riscLowerMisplacedAddresses() to call a riscLowerMisplacedAddressesInInstruction() which actually does the "lowering" for a single instruction instead of a list of instructions.  
> 3. In mips.rb, call a mipsLowerMisplacedAddresses() instead of riscLowerMisplacedAddresses().
> 4. In mipsLowerMisplacedAddresses(),
>     3.1 Handle the instructions that are specific to mips i.e. overridden behavior.
>     3.2 For instructions that the mips port does not wish to override, call down to riscLowerMisplacedAddressesInInstruction() instead of doing the default "newList << node".
> 
> With this approach, a platform specific port gets to override the instructions it cares about, and reuse the rest.  And there is no need to determine ahead of time which instruction is common / specific.  Can you do this refactor please?
> 
> Other than that, everything else looks good to me.  Thanks.

I agree with Mark that your use of the terms "Common" and "Specific" is confusing.  I don't know what "common" or "specific" means (seriously, I don't - they're really uninformative words and could mean anything).  While the refactoring that Mark suggests would work, I would be happy with either of the following options:

1) Don't change risc.rb at all, and just call mipsLowerMisplacedAddressesSpecific before calling riskLowerMisplacedAddresses.  Do you think you could make that work?  It would clearly be less intrusive (which I like) but you don't have to go this route if you can't easily make it work.

2) Just use better names than "Common" and "Specific".  Instead of "riscLowerMisplacedAddressesSpecific" I would say "riscLowerMisplacedAddressesForJumpsAndCalls".  Instead of "riscLowerMisplacedAddressesCommon", I would say "riscLowerMisplacedAddressesForEverythingElse".  "ForEverythingElse" has the benefit of alerting the person reading the code to the fact that there are other variants of riscLowerMisplacedAddresses that do things for some other types of instructions.  It's better than "common" because there's no risk of people assuming that "common" subsumes "specific".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


More information about the webkit-unassigned mailing list