Support INIT_THIN_INLINE in LSubstr

NEW
Assigned to

Status

()

Core
JavaScript Engine: JIT
P5
normal
3 years ago
a year ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
bug 1119081 comment 5

Note: I tried to use CreateDependentString in LSubstr when implementing. The only issue is that LSubstr is very constrained with it registers making it possible to squeeze some more perf. out of it. Using CreateDependentString would mean introducing an push/pop to create an extra register (in x86, since we are out of registers with sps enabled).
(Assignee)

Comment 1

3 years ago
Created attachment 8574692 [details] [diff] [review]
Patch
Assignee: nobody → hv1989
Attachment #8574692 - Flags: review?(jdemooij)
(Assignee)

Comment 2

3 years ago
Comment on attachment 8574692 [details] [diff] [review]
Patch

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

I think I made a minor mistake. So clearing review for now.
Attachment #8574692 - Flags: review?(jdemooij)
(Assignee)

Comment 3

3 years ago
Created attachment 8574724 [details] [diff] [review]
Patch

Test for INIT_FAT_INLINE_FLAGS instead of INIT_THIN_INLINE_FLAGS, since INIT_FAT_INLINE_FLAGS = INIT_THIN_INLINE_FLAGS | JS_BIT(4)
Attachment #8574692 - Attachment is obsolete: true
Attachment #8574724 - Flags: review?(jdemooij)
Comment on attachment 8574724 [details] [diff] [review]
Patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +5884,5 @@
>      masm.branchTest32(Assembler::Zero, stringFlags, Imm32(JSString::INLINE_CHARS_BIT), &notInline);
> +
> +    Label createThinInline, next;
> +    masm.branchTest32(Assembler::Zero, stringFlags, Imm32(JSString::INIT_FAT_INLINE_FLAGS),
> +                      &createThinInline);

If we have a thin inline string, this will be NonZero as well because some of the flags in INIT_FAT_INLINE_FLAGS will be set. I think it'd be nice to use the length-register, because a substring of a non-inline string will often fit in an inline string and a FatInlineString will often have a ThinInlineString substring. I didn't check but I expect that's what the C++ code does too.
Attachment #8574724 - Flags: review?(jdemooij)
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.