Closed Bug 964229 Opened 6 years ago Closed 6 years ago

IonMonkey: Make sure re.exec() can get replaced with MRegExpExec in octane-regexp

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files, 3 obsolete files)

1) Add path for MIRType_Value in MToString (when definitely not an object)
2) Add paths for MIRType_Undefined, MIRType_Null, MIRType_Boolean to MToString
3) Fix StringPolicy and add ConvertToStringPolicy for when MToString can be used (i.e. where the spec says ToString is used on input).
4) Add type barriers on result of re.exec().

=> this will allow us to replace re.exec() in octane-regexp with MRegExpExec. Currently bug 951439 doesn't work because of this. (bug 951439 comment 10 explains more). And also will remove the possible regression!
Assignee: nobody → hv1989
Attachment #8365929 - Flags: review?(jdemooij)
Attachment #8365930 - Flags: review?(sstangl)
Comment on attachment 8365929 [details] [diff] [review]
Fix StringPolicy + improve MToString to support all primitives

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

::: js/src/jit/CodeGenerator.cpp
@@ +792,5 @@
>                                     StoreRegisterTo(output));
>      if (!ool)
>          return false;
>  
> +    // Try double to integer converstion and run integer to string code.

"converstion"
Comment on attachment 8365929 [details] [diff] [review]
Fix StringPolicy + improve MToString to support all primitives

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

Clearing review. Gonna upload a new patch in a second with testcases and a bug less ;)

::: js/src/jit/BaselineBailouts.cpp
@@ +1255,5 @@
>          return BAILOUT_RETURN_FATAL_ERROR;
>      IonSpew(IonSpew_BaselineBailouts, "  Incoming frame ptr = %p", builder.startFrame());
>  
>      SnapshotIterator snapIter(iter);
> +    snapIter.spewBailingFrom();

Oops. This shouldn't have been in this patch!

::: js/src/jit/CodeGenerator.cpp
@@ +792,5 @@
>                                     StoreRegisterTo(output));
>      if (!ool)
>          return false;
>  
> +    // Try double to integer converstion and run integer to string code.

s/converstion/conversion/
Attachment #8365929 - Flags: review?(jdemooij)
Attachment #8365929 - Attachment is obsolete: true
Attachment #8365961 - Flags: review?(jdemooij)
Attached patch Add testcase (obsolete) — Splinter Review
Forgot to add testcase. So adding separately now. No need for review on testcase. So r+ myself
Attachment #8365966 - Flags: review+
Comment on attachment 8365961 [details] [diff] [review]
Fix StringPolicy + improve MToString to support all primitives

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

Great work. I think you can use LPointer instead of LUndefinedString/LNullString; will re-review quickly with that change.

::: js/src/jit/CodeGenerator.cpp
@@ +747,5 @@
> +    // Fast path for small integers.
> +    masm.movePtr(ImmPtr(&GetIonContext()->runtime->staticStrings().intStaticTable), output);
> +    masm.loadPtr(BaseIndex(output, input, ScalePointer), output);
> +
> +    return true;

Nit: maybe make this return void so that the callers don't need the return-value-check.

@@ +850,5 @@
> +        masm.branchTestDouble(Assembler::NotEqual, tag, &notDouble);
> +        // Note: no fastpath. Need two extra registers and can only convert doubles
> +        // that fit integers and are smaller than StaticStrings::INT_STATIC_LIMIT.
> +        masm.jump(ool->entry());
> +        masm.bind(&notDouble);

Nit: masm.branchTestDouble(Assembler::Equal, tag, ool->entry());

::: js/src/jit/Lowering.cpp
@@ +1822,5 @@
> +      }
> +
> +      case MIRType_Undefined: {
> +        LUndefinedString *lir = new(alloc()) LUndefinedString();
> +        return define(lir, ins);

I think you can use LPointer(bla, LPointer::NON_GC_THING) instead of LUndefinedString and LNullString.
Attachment #8365961 - Flags: review?(jdemooij)
Attachment #8365930 - Flags: review?(sstangl) → review+
You requested "LPointer(bla, LPointer::NON_GC_THING)", but I did only LPointer(bla). We use ImmGCPtr for these everywhere. So if you think I should use "LPointer::NON_GC_THING", I should change all uses also to ImmPtr(bla) instead of ImmGCptr(bla) ...
Attachment #8365961 - Attachment is obsolete: true
Attachment #8365966 - Attachment is obsolete: true
Attachment #8367911 - Flags: review?(jdemooij)
Comment on attachment 8367911 [details] [diff] [review]
Fix StringPolicy + improve MToString to support all primitives

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

(In reply to Hannes Verschore [:h4writer] from comment #8)
> You requested "LPointer(bla, LPointer::NON_GC_THING)", but I did only
> LPointer(bla). We use ImmGCPtr for these everywhere.

Sorry, you're right; don't know what I was thinking.
Attachment #8367911 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/797a3c64b400
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 966926
You need to log in before you can comment on or make changes to this bug.