1446061 - Follow-up from bug 1438842 review comments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[André Bargull [:anba]]

Follow-up from Ted's review comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1438842#c11.

Comment 1

[André Bargull [:anba]]

Attachment: bug1446061.patch

Only f? because I'm not sure how to fix two of the follow-up suggestions.


From https://bugzilla.mozilla.org/show_bug.cgi?id=1438842#c11

> > ::: js/src/jit/BaselineBailouts.cpp
> Follow-up: we should remove the local caller, callerPC variables and pass frameNo instead.

InitFromBailout now uses |frameNo| instead of |callerPC|. The |callPC| out-param is also no longer used, except for assertions, so we can remove it, too. The assertion now uses |nextCallee| which should have the same effect.


> > ::: js/src/jit/BaselineCompiler.cpp
> Follow-up: CodeLocationLabel::repoint should be fixed so IonScript::copyICEntries can remove the masm arg as well.

From http://en.cppreference.com/w/cpp/types/size_t
> On many platforms (an exception is systems with segmented addressing) std::size_t can safely store the value of any non-member pointer, in which case it is synonymous with std::uintptr_t. 

Does that mean these two lines [1] are basically equivalent on all platforms we support? And if that's the case, currently the only effect of passing |MacroAssembler*| to repoint() is running this assertion [2], right? So, do we want to keep this assertion and conditionally test it or can it be removed?

[1] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/js/src/jit/shared/Assembler-shared.cpp#41,46
[2] https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/js/src/jit/shared/Assembler-shared.cpp#43-45


> > ::: js/src/jit/BaselineInspector.cpp
> Follow-up: |if (innerized) return false;|

I don't know where this statement should be added.


> > ::: js/src/jit/shared/BaselineCompiler-shared.cpp
> Follow-up: Remove ionOSRCompileable_

This one was already fixed in https://hg.mozilla.org/mozilla-central/rev/3ac5bb463708.

Comment 2

[Ted Campbell [:tcampbell]]

Thanks for doing this :)

(In reply to André Bargull [:anba] from comment #1)

For the repoint() case, let's just use uintptr_t (which is more correct since base type is a pointer). I think let's drop that wierd UINT32_MAX assertion. This code was inlined so many times through API changes that it barely makes sense anymore.

> > > ::: js/src/jit/BaselineInspector.cpp
> > Follow-up: |if (innerized) return false;|
> 
> I don't know where this statement should be added.

This can be the first active line of AddCacheIRGlobalGetter. As in, we don't recognize the IC as something we support if marked as innerized and therefore stop trying to match and do a return instead.

Comment 3

[Ted Campbell [:tcampbell]]

Comment on attachment 8959252 [details] [diff] [review]
bug1446061.patch

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

Let's drop the assertInBounds stuff.

::: js/src/jit/shared/Assembler-shared.cpp
@@ +43,2 @@
>  #ifdef JS_CODEGEN_X64
>          MOZ_ASSERT((uint64_t)raw_ <= UINT32_MAX);

Let's just remove the assert. It is bizarre for it to be checked here.

Comment 4

[André Bargull [:anba]]

Attachment: bug1446061.patch

Thanks for the feedback comments!

Updated patch:
- Removed assertion from CodeLocationLabel::repoint().
- Added |if (innerized) return false| to AddCacheIRGlobalGetter().

Comment 5

[Ted Campbell [:tcampbell]]

Comment on attachment 8959292 [details] [diff] [review]
bug1446061.patch

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

Great.

::: js/src/jit/BaselineBailouts.cpp
@@ +826,5 @@
>          MOZ_ASSERT(iter.numAllocations() >= CountArgSlots(script, fun));
>          JitSpew(JitSpew_BaselineBailouts, "      frame slots %u, nargs %zu, nfixed %zu",
>                  iter.numAllocations(), fun->nargs(), script->nfixed());
>  
> +        if (frameNo == 0) {

Yay! I would get confused every time I looked at the old way.

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc081d096b578137acf279e1887e600d72b5ca46

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9066b0958419
Follow-up changes from unused parameter removal review. r=tcampbell

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9066b0958419