[webkit-reviews] review granted: [Bug 99706] MIPS LLInt implementation. : [Attachment 181313] LLInt implementation for MIPS reusing risc.rb.

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


Filip Pizlo <fpizlo at apple.com> has granted Balazs Kilvady
<kilvadyb at homejinni.com>'s request for review:
Bug 99706: MIPS LLInt implementation.
https://bugs.webkit.org/show_bug.cgi?id=99706

Attachment 181313: LLInt implementation for MIPS reusing risc.rb.
https://bugs.webkit.org/attachment.cgi?id=181313&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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".


More information about the webkit-reviews mailing list