Skip to content

Commit 9681601

Browse files
Georg SchmidCommit Bot
Georg Schmid
authored and
Commit Bot
committed
Reland "[csa] Tweak CSA pipeline to eliminate more redundant checks"
This is a reland of a66e3e5 Original change's description: > [csa] Tweak CSA pipeline to eliminate more redundant checks > > - Lower LoadObjectField to LoadFromObject > - Mark LoadFromObject and StoreToObject as non-allocating > - Use optimizable BitcastTaggedSignedToWord in TaggedIsNotSmi check > > R=jarin@chromium.org, tebbi@chromium.org > > Change-Id: I42992d46597be795aee3702018f7efd93fcc6ebf > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1657926 > Commit-Queue: Georg Schmid <gsps@google.com> > Reviewed-by: Tobias Tebbi <tebbi@chromium.org> > Cr-Commit-Position: refs/heads/master@{#62173} R=tebbi@chromium.org Change-Id: Id7ae13ba17a2083fd4109f34ce026030716ececb Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1660622 Commit-Queue: Georg Schmid <gsps@google.com> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Cr-Commit-Position: refs/heads/master@{#62202}
1 parent c51e4f3 commit 9681601

9 files changed

Lines changed: 126 additions & 24 deletions

File tree

‎src/builtins/base.tq‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3017,3 +3017,8 @@ transitioning macro ToStringImpl(context: Context, o: Object): String {
30173017
}
30183018
unreachable;
30193019
}
3020+
3021+
macro VerifiedUnreachable(): never {
3022+
StaticAssert(false);
3023+
unreachable;
3024+
}

‎src/codegen/code-stub-assembler.cc‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -975,9 +975,12 @@ TNode<BoolT> CodeStubAssembler::TaggedIsSmi(TNode<MaybeObject> a) {
975975
}
976976

977977
TNode<BoolT> CodeStubAssembler::TaggedIsNotSmi(SloppyTNode<Object> a) {
978-
return WordNotEqual(
979-
WordAnd(BitcastTaggedToWord(a), IntPtrConstant(kSmiTagMask)),
980-
IntPtrConstant(0));
978+
// Although BitcastTaggedSignedToWord is generally unsafe on HeapObjects, we
979+
// can nonetheless use it to inspect the Smi tag. The assumption here is that
980+
// the GC will not exchange Smis for HeapObjects or vice-versa.
981+
TNode<IntPtrT> a_bitcast = BitcastTaggedSignedToWord(UncheckedCast<Smi>(a));
982+
return WordNotEqual(WordAnd(a_bitcast, IntPtrConstant(kSmiTagMask)),
983+
IntPtrConstant(0));
981984
}
982985

983986
TNode<BoolT> CodeStubAssembler::TaggedIsPositiveSmi(SloppyTNode<Object> a) {
@@ -1394,14 +1397,15 @@ Node* CodeStubAssembler::LoadBufferObject(Node* buffer, int offset,
13941397
Node* CodeStubAssembler::LoadObjectField(SloppyTNode<HeapObject> object,
13951398
int offset, MachineType type) {
13961399
CSA_ASSERT(this, IsStrong(object));
1397-
return Load(type, object, IntPtrConstant(offset - kHeapObjectTag));
1400+
return LoadFromObject(type, object, IntPtrConstant(offset - kHeapObjectTag));
13981401
}
13991402

14001403
Node* CodeStubAssembler::LoadObjectField(SloppyTNode<HeapObject> object,
14011404
SloppyTNode<IntPtrT> offset,
14021405
MachineType type) {
14031406
CSA_ASSERT(this, IsStrong(object));
1404-
return Load(type, object, IntPtrSub(offset, IntPtrConstant(kHeapObjectTag)));
1407+
return LoadFromObject(type, object,
1408+
IntPtrSub(offset, IntPtrConstant(kHeapObjectTag)));
14051409
}
14061410

14071411
TNode<IntPtrT> CodeStubAssembler::LoadAndUntagObjectField(

‎src/compiler/memory-optimizer.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ bool CanAllocate(const Node* node) {
108108
case IrOpcode::kLoad:
109109
case IrOpcode::kLoadElement:
110110
case IrOpcode::kLoadField:
111+
case IrOpcode::kLoadFromObject:
111112
case IrOpcode::kPoisonedLoad:
112113
case IrOpcode::kProtectedLoad:
113114
case IrOpcode::kProtectedStore:
@@ -118,6 +119,7 @@ bool CanAllocate(const Node* node) {
118119
case IrOpcode::kStore:
119120
case IrOpcode::kStoreElement:
120121
case IrOpcode::kStoreField:
122+
case IrOpcode::kStoreToObject:
121123
case IrOpcode::kTaggedPoisonOnSpeculation:
122124
case IrOpcode::kUnalignedLoad:
123125
case IrOpcode::kUnalignedStore:

‎test/cctest/torque/test-torque.cc‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,36 @@ TEST(TestLoadEliminationVariableNoWrite) {
530530
asm_tester.GenerateCode();
531531
}
532532

533+
TEST(TestRedundantArrayElementCheck) {
534+
CcTest::InitializeVM();
535+
Isolate* isolate(CcTest::i_isolate());
536+
i::HandleScope scope(isolate);
537+
Handle<Context> context =
538+
Utils::OpenHandle(*v8::Isolate::GetCurrent()->GetCurrentContext());
539+
CodeAssemblerTester asm_tester(isolate);
540+
TestTorqueAssembler m(asm_tester.state());
541+
{
542+
m.Return(m.TestRedundantArrayElementCheck(
543+
m.UncheckedCast<Context>(m.HeapConstant(context))));
544+
}
545+
asm_tester.GenerateCode();
546+
}
547+
548+
TEST(TestRedundantSmiCheck) {
549+
CcTest::InitializeVM();
550+
Isolate* isolate(CcTest::i_isolate());
551+
i::HandleScope scope(isolate);
552+
Handle<Context> context =
553+
Utils::OpenHandle(*v8::Isolate::GetCurrent()->GetCurrentContext());
554+
CodeAssemblerTester asm_tester(isolate);
555+
TestTorqueAssembler m(asm_tester.state());
556+
{
557+
m.Return(m.TestRedundantSmiCheck(
558+
m.UncheckedCast<Context>(m.HeapConstant(context))));
559+
}
560+
asm_tester.GenerateCode();
561+
}
562+
533563
} // namespace compiler
534564
} // namespace internal
535565
} // namespace v8

‎test/torque/test-torque.tq‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -937,4 +937,34 @@ namespace test {
937937
StaticAssert(WordEqual(u1, u2));
938938
}
939939

940+
@export
941+
macro TestRedundantArrayElementCheck(implicit context: Context)(): Smi {
942+
const a = kEmptyFixedArray;
943+
for (let i: Smi = 0; i < a.length; i++) {
944+
if (a.objects[i] == Hole) {
945+
if (a.objects[i] == Hole) {
946+
return -1;
947+
} else {
948+
StaticAssert(false);
949+
}
950+
}
951+
}
952+
return 1;
953+
}
954+
955+
@export
956+
macro TestRedundantSmiCheck(implicit context: Context)(): Smi {
957+
const a = kEmptyFixedArray;
958+
const x = a.objects[1];
959+
typeswitch (x) {
960+
case (Smi): {
961+
Cast<Smi>(x) otherwise VerifiedUnreachable();
962+
return -1;
963+
}
964+
case (Object): {
965+
}
966+
}
967+
return 1;
968+
}
969+
940970
}

‎test/unittests/compiler/node-test-utils.cc‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,9 +1095,11 @@ class IsStoreElementMatcher final : public TestNodeMatcher {
10951095
if (NodeProperties::FirstControlIndex(node) < node->InputCount()) { \
10961096
control_node = NodeProperties::GetControlInput(node); \
10971097
} \
1098+
LoadRepresentation rep = IrOpcode::kLoadFromObject == node->opcode() \
1099+
? ObjectAccessOf(node->op()).machine_type \
1100+
: LoadRepresentationOf(node->op()); \
10981101
return (TestNodeMatcher::MatchAndExplain(node, listener) && \
1099-
PrintMatchAndExplain(LoadRepresentationOf(node->op()), "rep", \
1100-
rep_matcher_, listener) && \
1102+
PrintMatchAndExplain(rep, "rep", rep_matcher_, listener) && \
11011103
PrintMatchAndExplain(NodeProperties::GetValueInput(node, 0), \
11021104
"base", base_matcher_, listener) && \
11031105
PrintMatchAndExplain(NodeProperties::GetValueInput(node, 1), \
@@ -1119,6 +1121,7 @@ class IsStoreElementMatcher final : public TestNodeMatcher {
11191121
LOAD_MATCHER(Load)
11201122
LOAD_MATCHER(UnalignedLoad)
11211123
LOAD_MATCHER(PoisonedLoad)
1124+
LOAD_MATCHER(LoadFromObject)
11221125

11231126
#define STORE_MATCHER(kStore) \
11241127
class Is##kStore##Matcher final : public TestNodeMatcher { \
@@ -2037,6 +2040,16 @@ Matcher<Node*> IsUnalignedLoad(const Matcher<LoadRepresentation>& rep_matcher,
20372040
control_matcher));
20382041
}
20392042

2043+
Matcher<Node*> IsLoadFromObject(const Matcher<LoadRepresentation>& rep_matcher,
2044+
const Matcher<Node*>& base_matcher,
2045+
const Matcher<Node*>& index_matcher,
2046+
const Matcher<Node*>& effect_matcher,
2047+
const Matcher<Node*>& control_matcher) {
2048+
return MakeMatcher(new IsLoadFromObjectMatcher(rep_matcher, base_matcher,
2049+
index_matcher, effect_matcher,
2050+
control_matcher));
2051+
}
2052+
20402053
Matcher<Node*> IsStore(const Matcher<StoreRepresentation>& rep_matcher,
20412054
const Matcher<Node*>& base_matcher,
20422055
const Matcher<Node*>& index_matcher,

‎test/unittests/compiler/node-test-utils.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,11 @@ Matcher<Node*> IsUnalignedLoad(const Matcher<LoadRepresentation>& rep_matcher,
333333
const Matcher<Node*>& index_matcher,
334334
const Matcher<Node*>& effect_matcher,
335335
const Matcher<Node*>& control_matcher);
336+
Matcher<Node*> IsLoadFromObject(const Matcher<LoadRepresentation>& rep_matcher,
337+
const Matcher<Node*>& base_matcher,
338+
const Matcher<Node*>& index_matcher,
339+
const Matcher<Node*>& effect_matcher,
340+
const Matcher<Node*>& control_matcher);
336341
Matcher<Node*> IsStore(const Matcher<StoreRepresentation>& rep_matcher,
337342
const Matcher<Node*>& base_matcher,
338343
const Matcher<Node*>& index_matcher,

‎test/unittests/interpreter/interpreter-assembler-unittest.cc‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,15 @@ Matcher<Node*> InterpreterAssemblerTest::InterpreterAssemblerForTest::IsLoad(
6363
return ::i::compiler::IsLoad(rep_matcher, base_matcher, index_matcher, _, _);
6464
}
6565

66+
Matcher<Node*>
67+
InterpreterAssemblerTest::InterpreterAssemblerForTest::IsLoadFromObject(
68+
const Matcher<c::LoadRepresentation>& rep_matcher,
69+
const Matcher<Node*>& base_matcher, const Matcher<Node*>& index_matcher) {
70+
CHECK_NE(PoisoningMitigationLevel::kPoisonAll, poisoning_level());
71+
return ::i::compiler::IsLoadFromObject(rep_matcher, base_matcher,
72+
index_matcher, _, _);
73+
}
74+
6675
Matcher<Node*> InterpreterAssemblerTest::InterpreterAssemblerForTest::IsStore(
6776
const Matcher<c::StoreRepresentation>& rep_matcher,
6877
const Matcher<Node*>& base_matcher, const Matcher<Node*>& index_matcher,
@@ -436,7 +445,7 @@ TARGET_TEST_F(InterpreterAssemblerTest, LoadConstantPoolEntry) {
436445
Node* load_constant = m.LoadConstantPoolEntry(index);
437446
#ifdef V8_COMPRESS_POINTERS
438447
Matcher<Node*> constant_pool_matcher =
439-
IsChangeCompressedToTagged(m.IsLoad(
448+
IsChangeCompressedToTagged(m.IsLoadFromObject(
440449
MachineType::AnyCompressed(),
441450
c::IsParameter(InterpreterDispatchDescriptor::kBytecodeArray),
442451
c::IsIntPtrConstant(BytecodeArray::kConstantPoolOffset -
@@ -448,7 +457,7 @@ TARGET_TEST_F(InterpreterAssemblerTest, LoadConstantPoolEntry) {
448457
kHeapObjectTag),
449458
LoadSensitivity::kCritical)));
450459
#else
451-
Matcher<Node*> constant_pool_matcher = m.IsLoad(
460+
Matcher<Node*> constant_pool_matcher = m.IsLoadFromObject(
452461
MachineType::AnyTagged(),
453462
c::IsParameter(InterpreterDispatchDescriptor::kBytecodeArray),
454463
c::IsIntPtrConstant(BytecodeArray::kConstantPoolOffset -
@@ -466,7 +475,7 @@ TARGET_TEST_F(InterpreterAssemblerTest, LoadConstantPoolEntry) {
466475
Node* load_constant = m.LoadConstantPoolEntry(index);
467476
#if V8_COMPRESS_POINTERS
468477
Matcher<Node*> constant_pool_matcher =
469-
IsChangeCompressedToTagged(m.IsLoad(
478+
IsChangeCompressedToTagged(m.IsLoadFromObject(
470479
MachineType::AnyCompressed(),
471480
c::IsParameter(InterpreterDispatchDescriptor::kBytecodeArray),
472481
c::IsIntPtrConstant(BytecodeArray::kConstantPoolOffset -
@@ -480,7 +489,7 @@ TARGET_TEST_F(InterpreterAssemblerTest, LoadConstantPoolEntry) {
480489
c::IsWordShl(index, c::IsIntPtrConstant(kTaggedSizeLog2))),
481490
LoadSensitivity::kCritical)));
482491
#else
483-
Matcher<Node*> constant_pool_matcher = m.IsLoad(
492+
Matcher<Node*> constant_pool_matcher = m.IsLoadFromObject(
484493
MachineType::AnyTagged(),
485494
c::IsParameter(InterpreterDispatchDescriptor::kBytecodeArray),
486495
c::IsIntPtrConstant(BytecodeArray::kConstantPoolOffset -
@@ -506,13 +515,13 @@ TARGET_TEST_F(InterpreterAssemblerTest, LoadObjectField) {
506515
int offset = 16;
507516
Node* load_field = m.LoadObjectField(object, offset);
508517
#ifdef V8_COMPRESS_POINTERS
509-
EXPECT_THAT(load_field, IsChangeCompressedToTagged(m.IsLoad(
518+
EXPECT_THAT(load_field, IsChangeCompressedToTagged(m.IsLoadFromObject(
510519
MachineType::AnyCompressed(), object,
511520
c::IsIntPtrConstant(offset - kHeapObjectTag))));
512521
#else
513-
EXPECT_THAT(load_field,
514-
m.IsLoad(MachineType::AnyTagged(), object,
515-
c::IsIntPtrConstant(offset - kHeapObjectTag)));
522+
EXPECT_THAT(load_field, m.IsLoadFromObject(
523+
MachineType::AnyTagged(), object,
524+
c::IsIntPtrConstant(offset - kHeapObjectTag)));
516525
#endif
517526
}
518527
}
@@ -593,21 +602,21 @@ TARGET_TEST_F(InterpreterAssemblerTest, LoadFeedbackVector) {
593602
kSystemPointerSize)));
594603
#ifdef V8_COMPRESS_POINTERS
595604
Matcher<Node*> load_vector_cell_matcher = IsChangeCompressedToTagged(
596-
m.IsLoad(MachineType::AnyCompressed(), load_function_matcher,
597-
c::IsIntPtrConstant(JSFunction::kFeedbackCellOffset -
598-
kHeapObjectTag)));
605+
m.IsLoadFromObject(MachineType::AnyCompressed(), load_function_matcher,
606+
c::IsIntPtrConstant(JSFunction::kFeedbackCellOffset -
607+
kHeapObjectTag)));
599608
EXPECT_THAT(load_feedback_vector,
600-
IsChangeCompressedToTagged(m.IsLoad(
609+
IsChangeCompressedToTagged(m.IsLoadFromObject(
601610
MachineType::AnyCompressed(), load_vector_cell_matcher,
602611
c::IsIntPtrConstant(Cell::kValueOffset - kHeapObjectTag))));
603612
#else
604-
Matcher<Node*> load_vector_cell_matcher = m.IsLoad(
613+
Matcher<Node*> load_vector_cell_matcher = m.IsLoadFromObject(
605614
MachineType::AnyTagged(), load_function_matcher,
606615
c::IsIntPtrConstant(JSFunction::kFeedbackCellOffset - kHeapObjectTag));
607-
EXPECT_THAT(
608-
load_feedback_vector,
609-
m.IsLoad(MachineType::AnyTagged(), load_vector_cell_matcher,
610-
c::IsIntPtrConstant(Cell::kValueOffset - kHeapObjectTag)));
616+
EXPECT_THAT(load_feedback_vector,
617+
m.IsLoadFromObject(
618+
MachineType::AnyTagged(), load_vector_cell_matcher,
619+
c::IsIntPtrConstant(Cell::kValueOffset - kHeapObjectTag)));
611620
#endif
612621
}
613622
}

‎test/unittests/interpreter/interpreter-assembler-unittest.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@ class InterpreterAssemblerTest : public TestWithIsolateAndZone {
4444
const Matcher<compiler::Node*>& base_matcher,
4545
const Matcher<compiler::Node*>& index_matcher,
4646
LoadSensitivity needs_poisoning = LoadSensitivity::kSafe);
47+
Matcher<compiler::Node*> IsLoadFromObject(
48+
const Matcher<compiler::LoadRepresentation>& rep_matcher,
49+
const Matcher<compiler::Node*>& base_matcher,
50+
const Matcher<compiler::Node*>& index_matcher);
4751
Matcher<compiler::Node*> IsStore(
4852
const Matcher<compiler::StoreRepresentation>& rep_matcher,
4953
const Matcher<compiler::Node*>& base_matcher,

0 commit comments

Comments
 (0)