1812655 - Try to remove 'substring' selfhosted code

Status: NEW

(Core :: JavaScript Engine, task, P2)

Description

[Ted Campbell [:tcampbell]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1374011#c2

The substring code is complex right now. We should continue to investigate if there is a better way to structure this to improve performance and make inlining more consistent.

Comment 1

[André Bargull [:anba]]

Bug 1374011 predates current optimisations to fold away the various Math.min/max calls. Right now we often end up with only a single MMinMax instruction for the common case when String.prototype.{slice,substring,substr} is called with constant int32 inputs. Bug 1782771 has more detailed comments.

There can be some boxing overhead, because Warp doesn't know the exact type set for some operations. When this function is called from a context where str is a boxed Value, for example when called from Baseline code.

function f(str) {
  return str.substring(2);
}

Warp isn't able to remove the MToString operation from the ToString call. And Warp is also not able to remove the initial null/undefined check. Both operations aren't necessary, because there's a preceding MUnbox to guard that str is a primitive String. It looks like it's not too difficult to fix this: The ToString call is actually a JSOp::ToString operation, so when we attach an IC for JSOp::ToString which performs a CacheOp::GuardToString, Warp will be able to merge the initial MUnbox with the MUnbox from CacheOp::GuardToString. The comparison operations can be removed with a new optimisation pass, which updates the comparison inputs when there's a dominating MUnbox operation for the same input. Proof of concept here. After doing these two changes, Warp is able to optimise the String_substring code when calling str.substring(2) to a simple sequence of MStringLength, MMinMax, MSub, and MSubstr.

---

Hmm, the comments in bug 1374011 are actually a bit confusing to me:

https://bugzilla.mozilla.org/show_bug.cgi?id=1374011#c2 is actually referring to this Substring helper function, whereas the profile linked in https://bugzilla.mozilla.org/show_bug.cgi?id=1374011#c0 shows intrinsic_SubstringKernel when called from Baseline/Interpreter, which is kind of expected, because Baseline doesn't inline CacheOp::CallSubstringKernelResult.