Closed Bug 1407867 Opened 4 years ago Closed 4 years ago

mips64 save/restore register fp

Categories

(Core :: JavaScript Engine: JIT, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: yuyin-hf, Assigned: yuyin-hf)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38

Steps to reproduce:

pull gecko, change to branch inbound.




Actual results:

some jit-test failed on mips64.


Expected results:

all pass.
fp use as Assembler::FramePointer, Bug 1393723 save/restore it in trampoline only on mips32, add support on mips64.
Attachment #8917630 - Flags: review?(luke)
Attachment #8917630 - Flags: review?(lhansen)
Attachment #8917632 - Flags: review?(luke)
Attachment #8917632 - Flags: review?(lhansen)
sorry, two attach 8917630 and  8917632 are same, Submitted two times
Attachment #8917630 - Attachment is obsolete: true
Attachment #8917630 - Flags: review?(luke)
Attachment #8917630 - Flags: review?(lhansen)
Comment on attachment 8917632 [details] [diff] [review]
0001-MIPS64-Add-missing-save-restore-registe-fp.patch

Review of attachment 8917632 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +44,4 @@
>      double f25;
>      double f24;
>  
> +    uintptr_t align;

Unused declaration?
Attachment #8917632 - Flags: review?(luke)
Attachment #8917632 - Flags: review?(lhansen)
Attachment #8917632 - Flags: review+
Thank you.

I want change it to :

--------------
 MOZ_MAYBE_UNUSED uintptr_t align;
--------------

what about do you think.

(In reply to Lars T Hansen [:lth] from comment #5)
> Comment on attachment 8917632 [details] [diff] [review]
> 0001-MIPS64-Add-missing-save-restore-registe-fp.patch
> 
> Review of attachment 8917632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/Trampoline-mips64.cpp
> @@ +44,4 @@
> >      double f25;
> >      double f24;
> >  
> > +    uintptr_t align;
> 
> Unused declaration?
Whatever works for you is fine with me.  I was just curious about it because i didn't see a use of the variable in the patch.  But perhaps you are using it elsewhere and I'm just not aware of it?
Oh, I see - it is padding for the structure.  So you should ignore my comment.
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
thank you for review :)

(In reply to Lars T Hansen [:lth] from comment #8)
> Oh, I see - it is padding for the structure.  So you should ignore my
> comment.
Keywords: checkin-needed
Assignee: nobody → yuyin-hf
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2eb7a581bde
MIPS64: Add missing save/restore registe fp. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2eb7a581bde
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.