A64: Port LSeqStringSetChar optimizations from r16707 and r17521. (issue 141713009)

[ul...@chromium.org]

Reviewers: baptiste.afsa1, m.m.capewell,

Message:
PTAL


https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64.h
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64.h#newcode1477
src/a64/macro-assembler-a64.h:1477: Register scratch,
This is different from arm port, which takes value register here and
doesn't use it.

Description:
A64: Port LSeqStringSetChar optimizations from r16707 and r17521.

BUG=
TEST=mjsunit/lithium/SeqStringSetChar

Please review this at https://codereview.chromium.org/141713009/

SVN Base: https://v8.googlecode.com/svn/branches/experimental/a64

Affected files (+28, -24 lines):
M src/a64/full-codegen-a64.cc
M src/a64/lithium-codegen-a64.cc
M src/a64/macro-assembler-a64.h
M src/a64/macro-assembler-a64.cc


Index: src/a64/full-codegen-a64.cc
diff --git a/src/a64/full-codegen-a64.cc b/src/a64/full-codegen-a64.cc
index
dd745848b088df669d8ae9af121731d5cbdbf606..2dca236faf482a986d45f7027bc8506c45e8b710
100644
--- a/src/a64/full-codegen-a64.cc
+++ b/src/a64/full-codegen-a64.cc
@@ -3255,9 +3255,10 @@ void
FullCodeGenerator::EmitOneByteSeqStringSetChar(CallRuntime* expr) {
__ Throw(kNonSmiValue);
__ Throw(kNonSmiIndex);
__ Bind(&both_smis);
-
+ __ SmiUntag(index);
static const uint32_t one_byte_seq_type = kSeqStringTag |
kOneByteStringTag;
- __ EmitSeqStringSetCharCheck(string, index, one_byte_seq_type);
+ __ EmitSeqStringSetCharCheck(string, index, scratch,
one_byte_seq_type);
+ __ SmiTag(index);
}

__ Add(scratch, string, SeqOneByteString::kHeaderSize - kHeapObjectTag);
@@ -3288,9 +3289,10 @@ void
FullCodeGenerator::EmitTwoByteSeqStringSetChar(CallRuntime* expr) {
__ Throw(kNonSmiValue);
__ Throw(kNonSmiIndex);
__ Bind(&both_smis);
-
+ __ SmiUntag(index);
static const uint32_t two_byte_seq_type = kSeqStringTag |
kTwoByteStringTag;
- __ EmitSeqStringSetCharCheck(string, index, two_byte_seq_type);
+ __ EmitSeqStringSetCharCheck(string, index, scratch,
two_byte_seq_type);
+ __ SmiTag(index);
}

__ Add(scratch, string, SeqTwoByteString::kHeaderSize - kHeapObjectTag);
Index: src/a64/lithium-codegen-a64.cc
diff --git a/src/a64/lithium-codegen-a64.cc b/src/a64/lithium-codegen-a64.cc
index
5b7a71b55311bfbd4712718514c8db82dcf9d58c..7c8830f5f0b46335ce0a47637ea12cd5a603e14a
100644
--- a/src/a64/lithium-codegen-a64.cc
+++ b/src/a64/lithium-codegen-a64.cc
@@ -4571,30 +4571,26 @@ void
LCodeGen::DoSeqStringGetChar(LSeqStringGetChar* instr) {


void LCodeGen::DoSeqStringSetChar(LSeqStringSetChar* instr) {
- // TODO(all): Port ARM optimizations from r16707.
-
String::Encoding encoding = instr->encoding();
Register string = ToRegister(instr->string());
- Register index = ToRegister(instr->index());
Register value = ToRegister(instr->value());
Register temp = ToRegister(instr->temp());

if (FLAG_debug_code) {
- if (encoding == String::ONE_BYTE_ENCODING) {
- __ EmitSeqStringSetCharCheck(
- string, index, kSeqStringTag | kOneByteStringTag);
- } else {
- ASSERT(encoding == String::TWO_BYTE_ENCODING);
- __ EmitSeqStringSetCharCheck(
- string, index, kSeqStringTag | kTwoByteStringTag);
- }
+ Register index = ToRegister(instr->index());
+ static const uint32_t one_byte_seq_type = kSeqStringTag |
kOneByteStringTag;
+ static const uint32_t two_byte_seq_type = kSeqStringTag |
kTwoByteStringTag;
+ int encoding_mask =
+ instr->hydrogen()->encoding() == String::ONE_BYTE_ENCODING
+ ? one_byte_seq_type : two_byte_seq_type;
+ __ EmitSeqStringSetCharCheck(string, index, temp, encoding_mask);
}
-
- __ Add(temp, string, SeqString::kHeaderSize - kHeapObjectTag);
+ MemOperand operand =
+ BuildSeqStringOperand(string, temp, instr->index(), encoding);
if (encoding == String::ONE_BYTE_ENCODING) {
- __ Strb(value, MemOperand(temp, index));
+ __ Strb(value, operand);
} else {
- __ Strh(value, MemOperand(temp, index, LSL, 1));
+ __ Strh(value, operand);
}
}

Index: src/a64/macro-assembler-a64.cc
diff --git a/src/a64/macro-assembler-a64.cc b/src/a64/macro-assembler-a64.cc
index
7a245ad65ac776592afb2ad7e7e4a33ec65fd6f0..24c7689f7409b794912f8628f60a58ed8514db72
100644
--- a/src/a64/macro-assembler-a64.cc
+++ b/src/a64/macro-assembler-a64.cc
@@ -3621,29 +3621,34 @@ void MacroAssembler::IndexFromHash(Register hash,
Register index) {

void MacroAssembler::EmitSeqStringSetCharCheck(Register string,
Register index,
+ Register scratch,
uint32_t encoding_mask) {
- Register scratch = __ Tmp1();
ASSERT(!AreAliased(string, index, scratch));

- AssertSmi(index);
-
// Check that string is an object.
ThrowIfSmi(string, kNonObject);

// Check that string has an appropriate map.
Ldr(scratch, FieldMemOperand(string, HeapObject::kMapOffset));
Ldrb(scratch, FieldMemOperand(scratch, Map::kInstanceTypeOffset));
+
And(scratch, scratch, kStringRepresentationMask | kStringEncodingMask);
Cmp(scratch, encoding_mask);
ThrowIf(ne, kUnexpectedStringType);

- // Check that the index points inside the string.
+ // The index is assumed to be untagged coming in, tag it to compare with
the
+ // string length without using a temp register, it is restored at the
end of
+ // this function.
+ SmiTag(index);
+
Ldr(scratch, FieldMemOperand(string, String::kLengthOffset));
Cmp(index, scratch);
ThrowIf(ge, kIndexIsTooLarge);

Cmp(index, Operand(Smi::FromInt(0)));
ThrowIf(lt, kIndexIsNegative);
+
+ SmiUntag(index);
}


Index: src/a64/macro-assembler-a64.h
diff --git a/src/a64/macro-assembler-a64.h b/src/a64/macro-assembler-a64.h
index
b3047eee6cdce3b5fa149bbb10c1d7c1b56773dd..fe0a94407f38814ac686ca177d86c71627fe5bb8
100644
--- a/src/a64/macro-assembler-a64.h
+++ b/src/a64/macro-assembler-a64.h
@@ -1473,7 +1473,8 @@ class MacroAssembler : public Assembler {
// Inline caching support.

void EmitSeqStringSetCharCheck(Register string,
- Register index, // Smi
+ Register index,
+ Register scratch,
uint32_t encoding_mask);

// Generate code for checking access rights - used for security checks

[baptis...@arm.com]

I don't really like the fact that you have to untag/retag the index in
fullcodegen. IMO it would be better to add an extra argument to the
MacroAssembler function to specify whether the index is tag or untagged and
make
the function able to cope with both situations. What do you think?




https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64.cc#newcode3645
src/a64/macro-assembler-a64.cc:3645: Cmp(index, scratch);
You can avoid to Tag/Untag the index by untagging scratch here just for
the Cmp. Here it will be something similar to:
Cmp(index, Operand::UntagSmi(scratch))

It will make use of shifted register and will avoid adding extra
instructions.

https://codereview.chromium.org/141713009/

[ul...@chromium.org]

On 2014/02/06 14:52:07, baptiste.afsa1 wrote:
> I don't really like the fact that you have to untag/retag the index in
> fullcodegen. IMO it would be better to add an extra argument to the
> MacroAssembler function to specify whether the index is tag or untagged
> and
make
> the function able to cope with both situations. What do you think?

Good idea, I uploaded a patch set that makes the function handle both cases.
PTAL.

https://codereview.chromium.org/141713009/

[baptis...@arm.com]

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.cc#newcode3648
src/a64/macro-assembler-a64.cc:3648: Cmp(index,
Operand(Smi::FromInt(0)));
Although it will work, it doesn't really make sense to compare with
Smi::FromInt(0) when the index is not a SMI.

Maybe you should compare with 0 and assert that Smi::FromInt(0) == 0
with a comment explaining what's going on here.

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.h
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.h#newcode1479
src/a64/macro-assembler-a64.h:1479: bool index_is_smi,
Usually v8 functions which use that kind of trick use an enum to specify
the "mode" instead of a bool. Maybe you should do the same here.

https://codereview.chromium.org/141713009/

[ul...@chromium.org]

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.cc#newcode3648
src/a64/macro-assembler-a64.cc:3648: Cmp(index,
Operand(Smi::FromInt(0)));

On 2014/02/06 15:48:13, baptiste.afsa1 wrote:
> Although it will work, it doesn't really make sense to compare with
> Smi::FromInt(0) when the index is not a SMI.

> Maybe you should compare with 0 and assert that Smi::FromInt(0) == 0
with a
> comment explaining what's going on here.

I added assert, but didn't add comment since the assert and the code
above makes it clear.

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.h
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler-a64.h#newcode1479
src/a64/macro-assembler-a64.h:1479: bool index_is_smi,

On 2014/02/06 15:48:13, baptiste.afsa1 wrote:
> Usually v8 functions which use that kind of trick use an enum to
specify the
> "mode" instead of a bool. Maybe you should do the same here.

Done.

https://codereview.chromium.org/141713009/

[baptis...@arm.com]

lgtm



https://codereview.chromium.org/141713009/

[ul...@chromium.org]

Committed patchset #4 manually as r19166 (presubmit successful).

https://codereview.chromium.org/141713009/