Closed Bug 1237284 Opened 4 years ago Closed 4 years ago

SIMD: Cleanup MCallOptimize

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

Details

Attachments

(2 files)

Some functions are called inlineSIMD* and other are called inline*SIMD. Naming matters.
This makes all SIMD inlining functions such that they have the inlineSimd[Name]
format. There are two inlineSimdCheck functions with this patch (one for the
actual "check" operation and another one to just check the argument's type),
although they have different signatures so they can't be confused for the
compiler.

Review commit: https://reviewboard.mozilla.org/r/29693/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29693/
Attachment #8704652 - Flags: review?(jolesen)
By removing the type argument of inlineSimdCheck, which is used only for an
assertion that never fails/failed, we can simplify inlineSimd* functions to
avoid one indirection, using directly MIRType arguments instead of
SimdTypeDescr::Type.

Review commit: https://reviewboard.mozilla.org/r/29695/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29695/
Attachment #8704653 - Flags: review?(jolesen)
Comment on attachment 8704653 [details]
MozReview Request: Bug 1237284: Make inlineSimd* functions take MIRType to avoid an indirection; r=jolesen

https://reviewboard.mozilla.org/r/29695/#review26549

Looks good!
Attachment #8704653 - Flags: review?(jolesen) → review+
Comment on attachment 8704652 [details]
MozReview Request: Bug 1237284: Make SIMD names more consistent in MCallOptimize; r=jolesen

https://reviewboard.mozilla.org/r/29693/#review26545

Looks good.

::: js/src/jit/IonBuilder.h:857
(Diff revision 1)
> -    bool checkInlineSimd(CallInfo& callInfo, JSNative native, SimdTypeDescr::Type type,
> +    bool inlineSimdCheck(CallInfo& callInfo, JSNative native, SimdTypeDescr::Type type,

I am not convinced that this function should be renamed to the same scheme. All the other functions renamed to `inlineSimd...()` actually perform inlining and return an `InlineStatus`, while this function performs checking and retuns a `bool`.

The first verb in the function name should describe what the function does.
Attachment #8704652 - Flags: review?(jolesen) → review+
https://reviewboard.mozilla.org/r/29693/#review26545

> I am not convinced that this function should be renamed to the same scheme. All the other functions renamed to `inlineSimd...()` actually perform inlining and return an `InlineStatus`, while this function performs checking and retuns a `bool`.
> 
> The first verb in the function name should describe what the function does.

Indeed. I fooled myself when renaming this function (that is, checkInlineSimd): it's not inlining a check at all, it's just checking preconditions for inlining a SIMD function. I renamed it "canInlineSimd" as it's returning a bool, and everytime it returns false we return InliningStatus_NotInlined. Let me know if that is fine for you or if you have a better name in mind (I'll push later today).
Comment on attachment 8704652 [details]
MozReview Request: Bug 1237284: Make SIMD names more consistent in MCallOptimize; r=jolesen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29693/diff/1-2/
Attachment #8704652 - Attachment description: MozReview Request: Bug 1237284: Make SIMD names more consistent in MCallOptimize; r?jolesen → MozReview Request: Bug 1237284: Make SIMD names more consistent in MCallOptimize; r=jolesen
Comment on attachment 8704653 [details]
MozReview Request: Bug 1237284: Make inlineSimd* functions take MIRType to avoid an indirection; r=jolesen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29695/diff/1-2/
Attachment #8704653 - Attachment description: MozReview Request: Bug 1237284: Make inlineSimd* functions take MIRType to avoid an indirection; r?jolesen → MozReview Request: Bug 1237284: Make inlineSimd* functions take MIRType to avoid an indirection; r=jolesen
(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> https://reviewboard.mozilla.org/r/29693/#review26545
> 
> I renamed it "canInlineSimd" as
> it's returning a bool, and everytime it returns false we return
> InliningStatus_NotInlined. Let me know if that is fine for you or if you
> have a better name in mind (I'll push later today).

Sounds great! Thanks.
https://hg.mozilla.org/mozilla-central/rev/888d04815b8b
https://hg.mozilla.org/mozilla-central/rev/aaab4a60fca8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.