[ic] Clean up handler boilerplate

[ic] Clean up handler boilerplate

- builtins-ic.cc takes the place of the AccessorAssembler shim
- AccessorAssemblerImpl can then be renamed
- some cleanup in code-factory.cc
- drop old _TF name suffixes
- fix Generate##Name##Impl in TF_BUILTIN macro

Review-Url: https://codereview.chromium.org/2647493002
Cr-Commit-Position: refs/heads/master@{#42520}
Committed: https://chromium.googlesource.com/v8/v8/+/89f5efb761a8d60521b660a3f8330e2b44a93938

[Jakob Kummerow, 2017-01-18 18:45:30]

jkummerow@chromium.org changed reviewers:
+ ishell@chromium.org

[Jakob Kummerow, 2017-01-18 18:45:31]

As discussed. PTAL.

https://codereview.chromium.org/2647493002/diff/1/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2647493002/diff/1/src/builtins/builtins.h#new...
src/builtins/builtins.h:215: TFS(LoadICProtoArray, HANDLER, Code::LOAD_IC,
LoadICProtoArray)              \
LoadICProtoArray must be declared before KeyedLoadIC_Megamorphic because the
latter calls the former.

https://codereview.chromium.org/2647493002/diff/1/src/ic/accessor-assembler.h
File src/ic/accessor-assembler.h (right):

https://codereview.chromium.org/2647493002/diff/1/src/ic/accessor-assembler.h...
src/ic/accessor-assembler.h:19: class AccessorAssembler : public
CodeStubAssembler {
All this is just moved over from accessor-assembler-impl.h.

[Igor Sheludko, 2017-01-18 20:32:55]

Nice! lgtm with a nit:

https://codereview.chromium.org/2647493002/diff/20001/src/ic/accessor-assembl...
File src/ic/accessor-assembler.h (right):

https://codereview.chromium.org/2647493002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.h:17: using compiler::Node;
It's bad practice to add using declarations into the header files because of
namespace pollution.
I think you can drop it and add a 
  typedef compiler::Node Node;
to the class definition instead if the similar definition existing in
CodeStubAssembler is not enough.

[Jakob Kummerow, 2017-01-19 10:14:40]

Thanks, landing.

https://codereview.chromium.org/2647493002/diff/20001/src/ic/accessor-assembl...
File src/ic/accessor-assembler.h (right):

https://codereview.chromium.org/2647493002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.h:17: using compiler::Node;
On 2017/01/18 20:32:55, Igor Sheludko wrote:
> It's bad practice to add using declarations into the header files because of
> namespace pollution.
> I think you can drop it and add a 
>   typedef compiler::Node Node;
> to the class definition instead if the similar definition existing in
> CodeStubAssembler is not enough.

Done. (I need both a typedef in the class, and "using compiler::Node" in the .cc
file.)

[Jakob Kummerow, 2017-01-19 14:54:38]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-19 14:54:38]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2647493002/#ps40001
(title: "fix nit")

[commit-bot: I haz the power, 2017-01-19 14:54:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 15:29:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 15:29:08]

Failed to apply patch for src/ic/accessor-assembler.cc:
While running git apply --index -p1;
  error: patch failed: src/ic/accessor-assembler.cc:1392
  error: src/ic/accessor-assembler.cc: patch does not apply

Patch:       src/ic/accessor-assembler.cc
Index: src/ic/accessor-assembler.cc
diff --git a/src/ic/accessor-assembler.cc b/src/ic/accessor-assembler.cc
index
43c269f9bf97d8b06818d21ce88f4ee57c9b856e..f4ef909b7f4d121e415f89c7519cc9be964c6da3
100644
--- a/src/ic/accessor-assembler.cc
+++ b/src/ic/accessor-assembler.cc
@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #include "src/ic/accessor-assembler.h"
-#include "src/ic/accessor-assembler-impl.h"
 
 #include "src/code-factory.h"
 #include "src/code-stubs.h"
@@ -14,14 +13,15 @@ namespace v8 {
 namespace internal {
 
 using compiler::CodeAssemblerState;
+using compiler::Node;
 
 //////////////////// Private helpers.
 
-Node* AccessorAssemblerImpl::TryMonomorphicCase(Node* slot, Node* vector,
-                                                Node* receiver_map,
-                                                Label* if_handler,
-                                                Variable* var_handler,
-                                                Label* if_miss) {
+Node* AccessorAssembler::TryMonomorphicCase(Node* slot, Node* vector,
+                                            Node* receiver_map,
+                                            Label* if_handler,
+                                            Variable* var_handler,
+                                            Label* if_miss) {
   Comment("TryMonomorphicCase");
   DCHECK_EQ(MachineRepresentation::kTagged, var_handler->rep());
 
@@ -51,9 +51,11 @@ Node* AccessorAssemblerImpl::TryMonomorphicCase(Node* slot,
Node* vector,
   return feedback;
 }
 
-void AccessorAssemblerImpl::HandlePolymorphicCase(
-    Node* receiver_map, Node* feedback, Label* if_handler,
-    Variable* var_handler, Label* if_miss, int unroll_count) {
+void AccessorAssembler::HandlePolymorphicCase(Node* receiver_map,
+                                              Node* feedback, Label*
if_handler,
+                                              Variable* var_handler,
+                                              Label* if_miss,
+                                              int unroll_count) {
   Comment("HandlePolymorphicCase");
   DCHECK_EQ(MachineRepresentation::kTagged, var_handler->rep());
 
@@ -98,7 +100,7 @@ void AccessorAssemblerImpl::HandlePolymorphicCase(
   Goto(if_miss);
 }
 
-void AccessorAssemblerImpl::HandleKeyedStorePolymorphicCase(
+void AccessorAssembler::HandleKeyedStorePolymorphicCase(
     Node* receiver_map, Node* feedback, Label* if_handler,
     Variable* var_handler, Label* if_transition_handler,
     Variable* var_transition_map_cell, Label* if_miss) {
@@ -135,7 +137,7 @@ void AccessorAssemblerImpl::HandleKeyedStorePolymorphicCase(
   Goto(if_miss);
 }
 
-void AccessorAssemblerImpl::HandleLoadICHandlerCase(
+void AccessorAssembler::HandleLoadICHandlerCase(
     const LoadICParameters* p, Node* handler, Label* miss,
     ElementSupport support_elements) {
   Comment("have_handler");
@@ -173,7 +175,7 @@ void AccessorAssemblerImpl::HandleLoadICHandlerCase(
   }
 }
 
-void AccessorAssemblerImpl::HandleLoadICSmiHandlerCase(
+void AccessorAssembler::HandleLoadICSmiHandlerCase(
     const LoadICParameters* p, Node* holder, Node* smi_handler, Label* miss,
     ElementSupport support_elements) {
   Variable var_double_value(this, MachineRepresentation::kFloat64);
@@ -295,7 +297,7 @@ void AccessorAssemblerImpl::HandleLoadICSmiHandlerCase(
   }
 }
 
-void AccessorAssemblerImpl::HandleLoadICProtoHandlerCase(
+void AccessorAssembler::HandleLoadICProtoHandlerCase(
     const LoadICParameters* p, Node* handler, Variable* var_holder,
     Variable* var_smi_handler, Label* if_smi_handler, Label* miss,
     bool throw_reference_error_if_nonexistent) {
@@ -368,15 +370,13 @@ void AccessorAssemblerImpl::HandleLoadICProtoHandlerCase(
 
   Bind(&array_handler);
   {
-    typedef LoadICProtoArrayDescriptor Descriptor;
-    LoadICProtoArrayStub stub(isolate(), throw_reference_error_if_nonexistent);
-    Node* target = HeapConstant(stub.GetCode());
-    TailCallStub(Descriptor(isolate()), target, p->context, p->receiver,
-                 p->name, p->slot, p->vector, handler);
+    TailCallStub(CodeFactory::LoadICProtoArray(
+                     isolate(), throw_reference_error_if_nonexistent),
+                 p->context, p->receiver, p->name, p->slot, p->vector,
handler);
   }
 }
 
-Node* AccessorAssemblerImpl::EmitLoadICProtoArrayCheck(
+Node* AccessorAssembler::EmitLoadICProtoArrayCheck(
     const LoadICParameters* p, Node* handler, Node* handler_length,
     Node* handler_flags, Label* miss,
     bool throw_reference_error_if_nonexistent) {
@@ -438,7 +438,7 @@ Node* AccessorAssemblerImpl::EmitLoadICProtoArrayCheck(
   return holder;
 }
 
-void AccessorAssemblerImpl::HandleLoadGlobalICHandlerCase(
+void AccessorAssembler::HandleLoadGlobalICHandlerCase(
     const LoadICParameters* pp, Node* handler, Label* miss,
     bool throw_reference_error_if_nonexistent) {
   LoadICParameters p = *pp;
@@ -457,7 +457,7 @@ void AccessorAssemblerImpl::HandleLoadGlobalICHandlerCase(
                              miss, kOnlyProperties);
 }
 
-void AccessorAssemblerImpl::HandleStoreICHandlerCase(
+void AccessorAssembler::HandleStoreICHandlerCase(
     const StoreICParameters* p, Node* handler, Label* miss,
     ElementSupport support_elements) {
   Label if_smi_handler(this), if_nonsmi_handler(this);
@@ -504,7 +504,7 @@ void AccessorAssemblerImpl::HandleStoreICHandlerCase(
   }
 }
 
-void AccessorAssemblerImpl::HandleStoreICElementHandlerCase(
+void AccessorAssembler::HandleStoreICElementHandlerCase(
     const StoreICParameters* p, Node* handler, Label* miss) {
   Comment("HandleStoreICElementHandlerCase");
   Node* validity_cell = LoadObjectField(handler, Tuple2::kValue1Offset);
@@ -521,8 +521,8 @@ void AccessorAssemblerImpl::HandleStoreICElementHandlerCase(
                p->value, p->slot, p->vector);
 }
 
-void AccessorAssemblerImpl::HandleStoreICProtoHandler(
-    const StoreICParameters* p, Node* handler, Label* miss) {
+void AccessorAssembler::HandleStoreICProtoHandler(const StoreICParameters* p,
+                                                  Node* handler, Label* miss) {
   // IC dispatchers rely on these assumptions to be held.
   STATIC_ASSERT(FixedArray::kLengthOffset ==
                 StoreHandler::kTransitionCellOffset);
@@ -613,11 +613,10 @@ void AccessorAssemblerImpl::HandleStoreICProtoHandler(
   }
 }
 
-void AccessorAssemblerImpl::HandleStoreICSmiHandlerCase(Node* handler_word,
-                                                        Node* holder,
-                                                        Node* value,
-                                                        Node* transition,
-                                                        Label* miss) {
+void AccessorAssembler::HandleStoreICSmiHandlerCase(Node* handler_word,
+                                                    Node* holder, Node* value,
+                                                    Node* transition,
+                                                    Label* miss) {
   Comment(transition ? "transitioning field store" : "field store");
 
 #ifdef DEBUG
@@ -683,9 +682,11 @@ void
AccessorAssemblerImpl::HandleStoreICSmiHandlerCase(Node* handler_word,
   }
 }
 
-void AccessorAssemblerImpl::HandleStoreFieldAndReturn(
-    Node* handler_word, Node* holder, Representation representation,
-    Node* value, Node* transition, Label* miss) {
+void AccessorAssembler::HandleStoreFieldAndReturn(Node* handler_word,
+                                                  Node* holder,
+                                                  Representation
representation,
+                                                  Node* value, Node*
transition,
+                                                  Label* miss) {
   bool transition_to_field = transition != nullptr;
   Node* prepared_value = PrepareValueForStore(
       handler_word, holder, representation, transition, value, miss);
@@ -727,11 +728,10 @@ void AccessorAssemblerImpl::HandleStoreFieldAndReturn(
   }
 }
 
-Node* AccessorAssemblerImpl::PrepareValueForStore(Node* handler_word,
-                                                  Node* holder,
-                                                  Representation
representation,
-                                                  Node* transition, Node*
value,
-                                                  Label* bailout) {
+Node* AccessorAssembler::PrepareValueForStore(Node* handler_word, Node* holder,
+                                              Representation representation,
+                                              Node* transition, Node* value,
+                                              Label* bailout) {
   if (representation.IsDouble()) {
     value = TryTaggedToFloat64(value, bailout);
 
@@ -762,7 +762,7 @@ Node* AccessorAssemblerImpl::PrepareValueForStore(Node*
handler_word,
   return value;
 }
 
-void AccessorAssemblerImpl::ExtendPropertiesBackingStore(Node* object) {
+void AccessorAssembler::ExtendPropertiesBackingStore(Node* object) {
   Node* properties = LoadProperties(object);
   Node* length = LoadFixedArrayBaseLength(properties);
 
@@ -798,11 +798,10 @@ void
AccessorAssemblerImpl::ExtendPropertiesBackingStore(Node* object) {
   StoreObjectField(object, JSObject::kPropertiesOffset, new_properties);
 }
 
-void AccessorAssemblerImpl::StoreNamedField(Node* handler_word, Node* object,
-                                            bool is_inobject,
-                                            Representation representation,
-                                            Node* value,
-                                            bool transition_to_field) {
+void AccessorAssembler::StoreNamedField(Node* handler_word, Node*…
(message too large)

[Jakob Kummerow, 2017-01-19 16:10:17]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-19 16:10:18]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2647493002/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2017-01-19 16:10:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 18:10:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 18:10:56]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[Jakob Kummerow, 2017-01-19 18:14:23]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2017-01-19 18:14:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 18:39:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484849663120370,
"parent_rev": "5e44cc79d54194267197fbe42e9a8b5e2b4d0249", "commit_rev":
"89f5efb761a8d60521b660a3f8330e2b44a93938"}

[commit-bot: I haz the power, 2017-01-19 18:39:12]

Description was changed from

==========
[ic] Clean up handler boilerplate

- builtins-ic.cc takes the place of the AccessorAssembler shim
- AccessorAssemblerImpl can then be renamed
- some cleanup in code-factory.cc
- drop old _TF name suffixes
- fix Generate##Name##Impl in TF_BUILTIN macro
==========

to

==========
[ic] Clean up handler boilerplate

- builtins-ic.cc takes the place of the AccessorAssembler shim
- AccessorAssemblerImpl can then be renamed
- some cleanup in code-factory.cc
- drop old _TF name suffixes
- fix Generate##Name##Impl in TF_BUILTIN macro

Review-Url: https://codereview.chromium.org/2647493002
Cr-Commit-Position: refs/heads/master@{#42520}
Committed:
https://chromium.googlesource.com/v8/v8/+/89f5efb761a8d60521b660a3f8330e2b44a...
==========

[commit-bot: I haz the power, 2017-01-19 18:39:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/89f5efb761a8d60521b660a3f8330e2b44a...