[Webkit-unassigned] [Bug 112886] [SH4] LLInt sh4 backend implementation

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from Filip Pizlo <fpizlo at apple.com>  2013-03-31 12:46:00 PST ---
(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 195893 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195893&action=review
> > 
> > I think this stills needs a bit of work.
> > 
> No problem, all comments are welcomed :)
> 
> > 
> > This comment should show an example of what kind of lowering happens.
> > 
> Sure, I'll add one.
> 
> 
> >
> > This seems strange.  This phase is identical to riscLowerMalformedAddressesDouble except that it changes 'loadd' and 'stored' to 'loadd+' and 'stored+'.  Except that you don't handle 'loadd' and 'stored' in the backend.  Why not just use riscLowerMalformedAddressesDouble and have the backend handle loadd and stored in the same way that you currently handle loadd+ and stored-?
> >
> I understand your point.
> The way I load or store a double for SH4 needs an Address type with a temporary register and a null offset, because I'll post-increment the address ('+') or pre-decrement ('-') the used register.
> If I use the riscLowerMalformedAddressesDouble, I won't be sure to get a temporary register for the Address, and it's not what I want because this register will be incremented by 8.
> That's why I used these "loadd+" and "stored-" to avoid confusion with a "regular" loadd or stored. Do you see a better way to proceed here ?

Intriguing, I missed that!

Let me try to distill my issues:

1) I don't like how sh4LowerMalformedAddressesDouble is named.  It's not lowering malformed addresses on double accesses.  Instead, it's lowering the access into a form where the address is available in a temporary so that you can emit an instruction that clobbers it.  I would call this phase sh4LowerDoubleAccesses, instead.

2) I'm not sure I like the naming of loadd+ and stored-.  Now that I understand their semantics, I think you're doing it right.  It is acceptable to give instructions long and verbose names, especially when their semantics are so weird.  I would call them loaddReversedAndIncrementAddress and storedReversedAndDecrementAddress.

3) Instead of emitting a addi in the stored case, you could have just created a new address with a different offset.  I think that would be cleaner.  In fact, this could be super useful in general so I would do it as follows: add a method called withOffset(integerValue) to Address, BaseIndex, and AbsoluteAddress.  Each of these will return a new version of themselves with the integerValue added to their preexisting offset.  Make sure that these methods don't clobber the receiver; offlineasm heavily relies on AST node instances being mostly immutable.

> 
> 
> >
> > It would have been _much_ better if you had modified the riscLowerMisplacedImmediates phase to either take a callback to determine wether the opcode needs such lowering, or to take a list or set of opcodes that require lowering.  As it stands you've duplicated the code and just changed the 'when' statement.
> >
> You're right, I'm going to follow one of your recommendation to avoid this code duplication.
> 
> 
> Thanks again for your comments, I'll submit a new patch soon.

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