Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1419373007:
[Interpreter] Adds implementation of bytecode graph builder for LoadICSloppy/Strict (Closed)
Description[Interpreter] Adds implementation of bytecode graph builder for LoadICSloppy/Strict.
Adds implementation and tests for following operators in bytecode graph builder:
-VisitLoadICSloppy
-VisitLoadICStrict
-VisitLoadICSloppyWide
-VisitLoadICStrictWide
The current implementation introduces empty frame states for frame state inputs expected by these operations.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/2acc2bc2a1a1dfc9d681e028a44f3657a99ebf1a
Cr-Commit-Position: refs/heads/master@{#32026}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Added unittests and addressed review comments #
Total comments: 41
Patch Set 3 : Addressed review comments #Patch Set 4 : removed extra spaces from code snippets in test-run-bytecode-graph-builder #
Total comments: 14
Patch Set 5 : Moved graph generation from bytecode into helper, so that all graph generation is at one place. #
Total comments: 22
Patch Set 6 : Removes FrameStateBeforeAndAfter and adds a function to replace framestate inputs with emtpy frame … #
Total comments: 13
Patch Set 7 : Addressed review comments #
Total comments: 18
Patch Set 8 : WIP: uploading so that other cls can be rebased #Patch Set 9 : Modified bytecode-graph-builder-unittest to build bytecode array instead of generating it. #
Dependent Patchsets: Messages
Total messages: 32 (8 generated)
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
Can you please review my changes. This cl is not complete yet. I have to add unit tests for LoadICSloppy and also tests for CreateObjectLiteral (both unit and integration). The implementation is ready for review. Thanks and Regards, Mythri https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:113: void AddToNode(Node* node) { In the AstGraphBuilder, this function expects two more parameters for constructing the frame state. I did not include them in this implementation. I am not sure if we need them or not. Actually, I did not completely understand what they are. I could look up and update the implementation/comment. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:210: void BytecodeGraphBuilder::PrepareFrameState(Node* node) { In the AstGraphBuilder, this function expects two more parameters. Same as FrameStateBeforeAndAfter::AddToNode https://codereview.chromium.org/1419373007/diff/1/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/1419373007/diff/1/src/compiler/pipeline.cc#ne... src/compiler/pipeline.cc:1065: !data.info()->shared_info()->HasBytecodeArray()) { If this phase is required, I think we have to add a similar one from bytecode. For now I just disabled it, when we are building the graph from bytecode array. I did not check where LoopAssignmentAnalsysis is used. This analysis crashes if we enable it when we are building it from bytecode. I think it is because it expects a AStGraph.
Looking good, a couple of comments to get started. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:104: // constructor. no need for this comment https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:107: // TODO(mythria) Replace with the actual frame state. Current /s/TODO(mythria)/TODO(mythria):/ (add the colon after the todo - throughout). https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:111: nit - remove extra newline (only a single line between class definition elements. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:120: // TODO(mythria) Replace with the actual frame state. Current nit - newline above https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:416: FrameStateBeforeAndAfter states(this); Add a DCHECK(is_sloppy(language_mode()) (and similarly is_strict() for the strict variants). https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:422: VectorSlotPair feedback = CreateVectorSlotPair(slot); Could you create a helper for the FeedbackVectorSlot slot = info()->feedback_vector()->ToSlot(iterator.GetIndexOperand(<operand>)); return CreateVectorSlotPair(slot); I have a feeling we will be doing this a lot. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:423: const Operator* op = javascript()->LoadNamed(language_mode(), name, feedback); nit - newline above https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:426: environment()->BindAccumulator(node); Could you pull this out into a BuildNamedLoad helper function, and have VisitLoadICStrict, VisitLoadICSloppyWide and VisitLoadICStrictWide also call this function. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:573: environment()->BindAccumulator(literal); Could you remove this and do it in a separate CL please. You can modify the test to pass an object to the function under test so that you don't need support for CreateObjectLiteral for the test. Also, please move BuildLoadObjectField to the other CL since this is only used in this visitor. https://codereview.chromium.org/1419373007/diff/1/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/1419373007/diff/1/src/compiler/pipeline.cc#ne... src/compiler/pipeline.cc:1065: !data.info()->shared_info()->HasBytecodeArray()) { On 2015/11/04 12:19:14, mythria wrote: > If this phase is required, I think we have to add a similar one from bytecode. > For now I just disabled it, when we are building the graph from bytecode array. > I did not check where LoopAssignmentAnalsysis is used. This analysis crashes if > we enable it when we are building it from bytecode. I think it is because it > expects a AStGraph. Could you dig into this a bit more and figure out what it needs - it is probably just that we aren't doing something we need to do in BytecodeGraphTester. https://codereview.chromium.org/1419373007/diff/1/src/interpreter/bytecode-ar... File src/interpreter/bytecode-array-builder.cc (right): https://codereview.chromium.org/1419373007/diff/1/src/interpreter/bytecode-ar... src/interpreter/bytecode-array-builder.cc:447: DCHECK(FitsInImm8Operand(flags)); // Flags should fit in 8 bits. Thanks :)
Could you please review my changes. I have a couple of more TODOs for adding correct effect and control for LoadICs, and another TODO in node-test-utils.cc to check if we have the expected typefeedback vector. I will work on them while you review my changes. Thanks, Mythri https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:104: // constructor. On 2015/11/04 14:27:37, rmcilroy wrote: > no need for this comment Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:107: // TODO(mythria) Replace with the actual frame state. Current On 2015/11/04 14:27:37, rmcilroy wrote: > /s/TODO(mythria)/TODO(mythria):/ (add the colon after the todo - throughout). Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:111: On 2015/11/04 14:27:37, rmcilroy wrote: > nit - remove extra newline (only a single line between class definition > elements. Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:120: // TODO(mythria) Replace with the actual frame state. Current On 2015/11/04 14:27:36, rmcilroy wrote: > nit - newline above Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:416: FrameStateBeforeAndAfter states(this); On 2015/11/04 14:27:37, rmcilroy wrote: > Add a DCHECK(is_sloppy(language_mode()) (and similarly is_strict() for the > strict variants). Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:422: VectorSlotPair feedback = CreateVectorSlotPair(slot); On 2015/11/04 14:27:37, rmcilroy wrote: > Could you create a helper for the > FeedbackVectorSlot slot = > info()->feedback_vector()->ToSlot(iterator.GetIndexOperand(<operand>)); > return CreateVectorSlotPair(slot); > > I have a feeling we will be doing this a lot. Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:426: environment()->BindAccumulator(node); On 2015/11/04 14:27:37, rmcilroy wrote: > Could you pull this out into a BuildNamedLoad helper function, and have > VisitLoadICStrict, VisitLoadICSloppyWide and VisitLoadICStrictWide also call > this function. Done. https://codereview.chromium.org/1419373007/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:573: environment()->BindAccumulator(literal); On 2015/11/04 14:27:37, rmcilroy wrote: > Could you remove this and do it in a separate CL please. You can modify the test > to pass an object to the function under test so that you don't need support for > CreateObjectLiteral for the test. > > Also, please move BuildLoadObjectField to the other CL since this is only used > in this visitor. Done.
Description was changed from ========== [Interpreter] Adds implementation of bytecode graph builder for LodICSloppy and CreateObjectLiteral. Adds implementation for VisitLoadICSloppy and VisitCreateObjectLiteral in bytecode graph builder. The current implementation introduces empty frame states for frame state inputs expected by these operations. Adds tests for VisitLoadICSloppy. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds implementation of bytecode graph builder for LodICSloppy/Strict. Adds implementation for VisitLoadICSloppy/Strict both for normal and wide modes in bytecode graph builder. The current implementation introduces empty frame states for frame state inputs expected by these operations. Adds tests for VisitLoadICSloppy. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Adds implementation of bytecode graph builder for LodICSloppy/Strict. Adds implementation for VisitLoadICSloppy/Strict both for normal and wide modes in bytecode graph builder. The current implementation introduces empty frame states for frame state inputs expected by these operations. Adds tests for VisitLoadICSloppy. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds implementation of bytecode graph builder for LoadICSloppy/Strict. Adds implementation for VisitLoadICSloppy/Strict both for normal and wide modes in bytecode graph builder. The current implementation introduces empty frame states for frame state inputs expected by these operations. Adds tests for VisitLoadICSloppy. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Adds implementation of bytecode graph builder for LoadICSloppy/Strict. Adds implementation for VisitLoadICSloppy/Strict both for normal and wide modes in bytecode graph builder. The current implementation introduces empty frame states for frame state inputs expected by these operations. Adds tests for VisitLoadICSloppy. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds implementation of bytecode graph builder for LoadICSloppy/Strict. Adds implementation and tests for following operators in bytecode graph builder: -VisitLoadICSloppy -VisitLoadICStrict -VisitLoadICSloppyWide -VisitLoadICStrictWide The current implementation introduces empty frame states for frame state inputs expected by these operations. BUG=v8:4280 LOG=N ==========
https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:200: VectorSlotPair BytecodeGraphBuilder::CreateVectorSlotPair( Just inline this into the function above (I don't see a situation where we would get a FeedbackVectorSlot without having first had to call ToSlot on an int). Also, when you do this pull out feedback_vector as a variable so that you get it once, rather than getting it from info and info->shared each time. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:206: // TODO(mythria): This implementation introduces empty frame state. Replace with nit - /empty frame state/an empty frame state/ https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:208: void BytecodeGraphBuilder::PrepareFrameState(Node* node) { Unused? If so, please remove for now. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:47: // Builders for accessing the function context. Fix comment https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:50: // Helper function for creating a pair of feedback vector and slot. /s/pair of feedback vector and slot./pair containing a type feedback vector and a feedback slot. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:51: // Named and keyed loads require a VectorSlotPair for successful lowering. no need for second line of comment (there are a bunch of other things which need feedback vector slot pairs) https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:114: // TODO(mythria): info() should not be used here. /s/info() should not be used here/Don't rely on parse information to get language mode. https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:18: nit - remove extra newline https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:148: REPEAT_4(SEP, __VA_ARGS__) SEP() REPEAT_2(SEP, __VA_ARGS__) SEP() __VA_ARGS__ These are all unused in this test now, right? If so please remove them for now. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:222: Matcher<Node*> BytecodeGraphBuilderTest::IsIntPtrConstant(bool is32, Don't pass is32 as a parameter, do in interpreter-assembler-unittest (kPointerSize == 8) https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:408: "function f(p1) { return p1.val; }; f( { val : 10 });"; nit - /f( {/f({/ (throughout CL) https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:413: int slot_id = slot.ToInt(); You shouldn't need the slot ids now you aren't matching on them https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:416: Node* ret = end->InputAt(0); nit - Node* ret = graph->end()->InputAt(0) https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:459: const char code_snippet[] = "function f(p1) { var b; " REPEAT_127( nit - could you space this so that the REPEAT_127 is on it's own line (like in the test below) https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... File test/unittests/compiler/node-test-utils.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1433: IsNamedLoadMatcher(IrOpcode::Value opcode, don't pass the opcode to this constructor, just use it directly below (if we want to later merge load and store we can do that but will also need to rename this class, so just fix it to be a load matcher for now. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1450: *os << ") feedback vector ("; /s/)/),/ https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1476: } Please use the same pattern as the other matchers were everthing is in the return statement and just uses PrintMatchAndExplain(...) && PrintMatchAndExplain(...) ... etc. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1492: class IsNamedAccessMatcher final : public MatcherInterface<NamedAccess> { As discussed, remove this. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:2109: Matcher<Node*> IsNamedLoad(const Matcher<NamedAccess>& node_access_matcher, IsJSLoadNamed (Follow the name of the IrOpcode) https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:2120: Matcher<NamedAccess> IsNamedAccess(LanguageMode language_mode, As discussed offline, let's not add a new NamedAccess matcher, instead just match on the "Matcher<Handle<Name>>& name". You can use StringTable::LookupString to find the canonical handle for a given string property name.
Can you please review my changes. Thanks, Mythri https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:200: VectorSlotPair BytecodeGraphBuilder::CreateVectorSlotPair( On 2015/11/09 15:23:00, rmcilroy wrote: > Just inline this into the function above (I don't see a situation where we would > get a FeedbackVectorSlot without having first had to call ToSlot on an int). > Also, when you do this pull out feedback_vector as a variable so that you get it > once, rather than getting it from info and info->shared each time. Done. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:206: // TODO(mythria): This implementation introduces empty frame state. Replace with On 2015/11/09 15:23:00, rmcilroy wrote: > nit - /empty frame state/an empty frame state/ Done. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:208: void BytecodeGraphBuilder::PrepareFrameState(Node* node) { On 2015/11/09 15:23:00, rmcilroy wrote: > Unused? If so, please remove for now. Done. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:47: // Builders for accessing the function context. On 2015/11/09 15:23:00, rmcilroy wrote: > Fix comment Done. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:50: // Helper function for creating a pair of feedback vector and slot. On 2015/11/09 15:23:00, rmcilroy wrote: > /s/pair of feedback vector and slot./pair containing a type feedback vector and > a feedback slot. Done. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:51: // Named and keyed loads require a VectorSlotPair for successful lowering. On 2015/11/09 15:23:00, rmcilroy wrote: > no need for second line of comment (there are a bunch of other things which need > feedback vector slot pairs) Done. https://codereview.chromium.org/1419373007/diff/20001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:114: // TODO(mythria): info() should not be used here. On 2015/11/09 15:23:00, rmcilroy wrote: > /s/info() should not be used here/Don't rely on parse information to get > language mode. Done. https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:18: On 2015/11/09 15:23:00, rmcilroy wrote: > nit - remove extra newline Done. https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:148: REPEAT_4(SEP, __VA_ARGS__) SEP() REPEAT_2(SEP, __VA_ARGS__) SEP() __VA_ARGS__ On 2015/11/09 15:23:00, rmcilroy wrote: > These are all unused in this test now, right? If so please remove them for now. I use them for testing LoadICS in wide mode in test BytecodeGraphBuilderNamedLoad. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:222: Matcher<Node*> BytecodeGraphBuilderTest::IsIntPtrConstant(bool is32, On 2015/11/09 15:23:00, rmcilroy wrote: > Don't pass is32 as a parameter, do in interpreter-assembler-unittest > (kPointerSize == 8) Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:408: "function f(p1) { return p1.val; }; f( { val : 10 });"; On 2015/11/09 15:23:00, rmcilroy wrote: > nit - /f( {/f({/ (throughout CL) Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:413: int slot_id = slot.ToInt(); On 2015/11/09 15:23:00, rmcilroy wrote: > You shouldn't need the slot ids now you aren't matching on them Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:416: Node* ret = end->InputAt(0); On 2015/11/09 15:23:00, rmcilroy wrote: > nit - Node* ret = graph->end()->InputAt(0) Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:459: const char code_snippet[] = "function f(p1) { var b; " REPEAT_127( On 2015/11/09 15:23:00, rmcilroy wrote: > nit - could you space this so that the REPEAT_127 is on it's own line (like in > the test below) Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... File test/unittests/compiler/node-test-utils.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1433: IsNamedLoadMatcher(IrOpcode::Value opcode, On 2015/11/09 15:23:01, rmcilroy wrote: > don't pass the opcode to this constructor, just use it directly below (if we > want to later merge load and store we can do that but will also need to rename > this class, so just fix it to be a load matcher for now. Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1450: *os << ") feedback vector ("; On 2015/11/09 15:23:01, rmcilroy wrote: > /s/)/),/ Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1476: } On 2015/11/09 15:23:01, rmcilroy wrote: > Please use the same pattern as the other matchers were everthing is in the > return statement and just uses PrintMatchAndExplain(...) && > PrintMatchAndExplain(...) ... etc. Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:1492: class IsNamedAccessMatcher final : public MatcherInterface<NamedAccess> { On 2015/11/09 15:23:01, rmcilroy wrote: > As discussed, remove this. Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:2109: Matcher<Node*> IsNamedLoad(const Matcher<NamedAccess>& node_access_matcher, On 2015/11/09 15:23:01, rmcilroy wrote: > IsJSLoadNamed (Follow the name of the IrOpcode) Done. https://codereview.chromium.org/1419373007/diff/20001/test/unittests/compiler... test/unittests/compiler/node-test-utils.cc:2120: Matcher<NamedAccess> IsNamedAccess(LanguageMode language_mode, On 2015/11/09 15:23:00, rmcilroy wrote: > As discussed offline, let's not add a new NamedAccess matcher, instead just > match on the "Matcher<Handle<Name>>& name". You can use > StringTable::LookupString to find the canonical handle for a given string > property name. As discussed, I pass Handle<Name> instead of matcher. If we need a matcher, then we can change it by implementing a matcher for Handle<Name>.
Looking really good. Final nits about the tests, but after they are fixed please add someone from the compiler team (Michi?) to review the compiler changes. https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:148: REPEAT_4(SEP, __VA_ARGS__) SEP() REPEAT_2(SEP, __VA_ARGS__) SEP() __VA_ARGS__ On 2015/11/10 09:55:35, mythria wrote: > On 2015/11/09 15:23:00, rmcilroy wrote: > > These are all unused in this test now, right? If so please remove them for > now. > > I use them for testing LoadICS in wide mode in test > BytecodeGraphBuilderNamedLoad. You're right, missed this. https://codereview.chromium.org/1419373007/diff/60001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/60001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:315: SPACE, " b = p1.name; ") "return p1.name;\n", nit - could you do the REPEAT_127(...) on one line please. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:59: class GraphGeneratorHelper { As discussed offline, pull the BytecodeArray code into this helper in order to share code (like InterpreterTester). https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:76: v8::Context::New(v8::Isolate::GetCurrent())->Enter(); nit - move this down to MakeGraph I think (you also need to exit the context - either in the destructor if you enter it here, or at the end of MakeGraph if you enter it there. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:79: Graph* MakeGraph(const char* script, const char* function_name) { nit - private https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:414: EXPECT_THAT(ret, IsReturn(IsJSLoadNamed(name, IsParameter(1), As discussed offline, pull this out into matchers to make it easier to read. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:444: "return p1.val;}; f({val:10});"; As discussed offline, let's make the repeated and wide property loads take different names to make it obvious which one we are matching on. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:450: isolate(), factory()->NewStringFromStaticChars("val")); nit - make a static helper for this in GraphGeneratorHelper::GetName (same as InterpreterTester::GetName()).
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org
Could you please review my changes. I added support for LoadICSloppy/Strict operators in bytecode graph builder and tests for them. Thanks, Mythri https://codereview.chromium.org/1419373007/diff/60001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/60001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:315: SPACE, " b = p1.name; ") "return p1.name;\n", On 2015/11/10 11:23:19, rmcilroy wrote: > nit - could you do the REPEAT_127(...) on one line please. Done. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:59: class GraphGeneratorHelper { On 2015/11/10 11:23:19, rmcilroy wrote: > As discussed offline, pull the BytecodeArray code into this helper in order to > share code (like InterpreterTester). Done. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:76: v8::Context::New(v8::Isolate::GetCurrent())->Enter(); On 2015/11/10 11:23:19, rmcilroy wrote: > nit - move this down to MakeGraph I think (you also need to exit the context - > either in the destructor if you enter it here, or at the end of MakeGraph if you > enter it there. Done. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:79: Graph* MakeGraph(const char* script, const char* function_name) { On 2015/11/10 11:23:19, rmcilroy wrote: > nit - private I removed this function after merging graph generation from bytecode into this helper. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:414: EXPECT_THAT(ret, IsReturn(IsJSLoadNamed(name, IsParameter(1), On 2015/11/10 11:23:19, rmcilroy wrote: > As discussed offline, pull this out into matchers to make it easier to read. Done. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:444: "return p1.val;}; f({val:10});"; On 2015/11/10 11:23:19, rmcilroy wrote: > As discussed offline, let's make the repeated and wide property loads take > different names to make it obvious which one we are matching on. Done. https://codereview.chromium.org/1419373007/diff/60001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:450: isolate(), factory()->NewStringFromStaticChars("val")); On 2015/11/10 11:23:19, rmcilroy wrote: > nit - make a static helper for this in GraphGeneratorHelper::GetName (same as > InterpreterTester::GetName()). Done.
LGTM with some final nits, thanks. https://codereview.chromium.org/1419373007/diff/80001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1419373007/diff/80001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:44: // Builders for accessing a (potentially immutable) object field. nit - you only have the immutable version here - update comment (until we add BuildLoadObjectField()) https://codereview.chromium.org/1419373007/diff/80001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:54: // Builds deoptimization for a given node. nit - not sure what you mean by "Builds deoptimization" here? https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:63: const char* function_name = kFunctionName) Do you think you'll ever need the function_name parameter here? I can't see a need, and would just remove it (we can always add it back later if need be). https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:69: const char* function_name = kFunctionName) No need for the function_name argument here - it'll never be exposed anyway for bytecode-array built functions. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:73: ParseInfo parse_info = GetParseInfo(); I'd just inline all of GetParseInfo() here and include the info.shared_info()->set_function_data... in the !script_ side - you're currently only saving 1 line of code by avoiding duplicating "CompilationInfo info(&parse_info);", but are adding 5 lines with the extra function, extra if, etc.). https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:102: const char* function_name_; Fields go below private functions: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:142: } else { DCHECK(!bytecode_.is_null()); https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:456: REPEAT_127(SPACE, " b = p1.name; ") nit - for clarity maybe make this val_prev or something (to avoid overloading "name" below) https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:466: Matcher<Node*> effect = IsJSLoadNamed( nit - call this preceeding_load or similar for clarity. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:477: REPEAT_127(SPACE, " b = p1.name; ") ditto https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:487: Matcher<Node*> effect = IsJSLoadNamed( ditto
As discussed in today's meeting, I removed FrameStateBeforeAndAfter abstraction. Now, I just replace frame state inputs with emtpy frame states. Could you please review my changes. Thanks, Mythri https://codereview.chromium.org/1419373007/diff/80001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1419373007/diff/80001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:44: // Builders for accessing a (potentially immutable) object field. On 2015/11/10 18:00:15, rmcilroy wrote: > nit - you only have the immutable version here - update comment (until we add > BuildLoadObjectField()) Done. https://codereview.chromium.org/1419373007/diff/80001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.h:54: // Builds deoptimization for a given node. On 2015/11/10 18:00:15, rmcilroy wrote: > nit - not sure what you mean by "Builds deoptimization" here? Removed this function as discussed in today's meeting https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:63: const char* function_name = kFunctionName) On 2015/11/10 18:00:15, rmcilroy wrote: > Do you think you'll ever need the function_name parameter here? I can't see a > need, and would just remove it (we can always add it back later if need be). Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:69: const char* function_name = kFunctionName) On 2015/11/10 18:00:15, rmcilroy wrote: > No need for the function_name argument here - it'll never be exposed anyway for > bytecode-array built functions. Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:73: ParseInfo parse_info = GetParseInfo(); On 2015/11/10 18:00:15, rmcilroy wrote: > I'd just inline all of GetParseInfo() here and include the > info.shared_info()->set_function_data... in the !script_ side - you're currently > only saving 1 line of code by avoiding duplicating "CompilationInfo > info(&parse_info);", but are adding 5 lines with the extra function, extra if, > etc.). I did this more to avoid using a new to create CompilationInfo. I wanted to use a stack variable to avoid deleting it explicitly. If I add CompilationInfo info(&parse_info) in both places, I have to pull the declaration of CompilationInfo to the top. We do not have correct parse_info then, so not all fields will get correctly initialized. Copy constructor are not allowed for CompilationInfo. So I can't do it like ParseInfo, where I create a new object and copy it. If it is better, I can switch to new and delete implementation. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:102: const char* function_name_; On 2015/11/10 18:00:15, rmcilroy wrote: > Fields go below private functions: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:142: } else { On 2015/11/10 18:00:15, rmcilroy wrote: > DCHECK(!bytecode_.is_null()); Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:456: REPEAT_127(SPACE, " b = p1.name; ") On 2015/11/10 18:00:15, rmcilroy wrote: > nit - for clarity maybe make this val_prev or something (to avoid overloading > "name" below) Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:466: Matcher<Node*> effect = IsJSLoadNamed( On 2015/11/10 18:00:15, rmcilroy wrote: > nit - call this preceeding_load or similar for clarity. Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:477: REPEAT_127(SPACE, " b = p1.name; ") On 2015/11/10 18:00:15, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1419373007/diff/80001/test/unittests/compiler... test/unittests/compiler/bytecode-graph-builder-unittest.cc:487: Matcher<Node*> effect = IsJSLoadNamed( On 2015/11/10 18:00:15, rmcilroy wrote: > ditto Done.
lgtm https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:357: void BytecodeGraphBuilder::AddFrameStateInputs(Node* node) { nit - move this up below CreateVectotSlotPair Try to keep related functionality clustered together (in this case all shared helper functions clustered, and bytecode visitors clustered below). https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.h:53: // Replaces the frame state inputs with frame states. /s/with frame states/with empty frame states https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.h:54: void AddFrameStateInputs(Node* node); nit - AddEmptyFrameStateInputs https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:87: ParseInfo p(zone_, function); don't use single letter variable names - also, no need for the extra variable, just do: parse_info = ParseInfo(zone_, shared_info); https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:100: ParseInfo p(zone_, shared_info); ditto
one more comment https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... File test/unittests/compiler/node-test-utils.cc (right): https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/node-test-utils.cc:1431: class IsNamedLoadMatcher final : public NodeMatcher { /s/IsNamedLoadMatcher/IsJsLoadNamedMatcher/
https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... File test/unittests/compiler/node-test-utils.cc (right): https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/node-test-utils.cc:1431: class IsNamedLoadMatcher final : public NodeMatcher { On 2015/11/11 14:23:51, rmcilroy wrote: > /s/IsNamedLoadMatcher/IsJsLoadNamedMatcher/ Actually to correct case: IsJSLoadNamedMatcher
addressed review comments from Ross. Thanks, Mythri https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:357: void BytecodeGraphBuilder::AddFrameStateInputs(Node* node) { On 2015/11/11 14:10:05, rmcilroy wrote: > nit - move this up below CreateVectotSlotPair > > Try to keep related functionality clustered together (in this case all shared > helper functions clustered, and bytecode visitors clustered below). Done. https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.h (right): https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.h:53: // Replaces the frame state inputs with frame states. On 2015/11/11 14:10:05, rmcilroy wrote: > /s/with frame states/with empty frame states Done. https://codereview.chromium.org/1419373007/diff/100001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.h:54: void AddFrameStateInputs(Node* node); On 2015/11/11 14:10:05, rmcilroy wrote: > nit - AddEmptyFrameStateInputs Done. https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:87: ParseInfo p(zone_, function); On 2015/11/11 14:10:06, rmcilroy wrote: > don't use single letter variable names - also, no need for the extra variable, > just do: > parse_info = ParseInfo(zone_, shared_info); Done. https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:100: ParseInfo p(zone_, shared_info); On 2015/11/11 14:10:06, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... File test/unittests/compiler/node-test-utils.cc (right): https://codereview.chromium.org/1419373007/diff/100001/test/unittests/compile... test/unittests/compiler/node-test-utils.cc:1431: class IsNamedLoadMatcher final : public NodeMatcher { On 2015/11/11 14:24:56, rmcilroy wrote: > On 2015/11/11 14:23:51, rmcilroy wrote: > > /s/IsNamedLoadMatcher/IsJsLoadNamedMatcher/ > > Actually to correct case: IsJSLoadNamedMatcher Done.
LGTM on the "compiler" piece in its current form. Only glossed over the rest.
https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:239: GraphGeneratorHelper helper(isolate(), zone(), It looks like the GraphGeneratorHelper is used in every unit test in this file. Is there a particular reason we couldn't just move everything into the BytecodeGraphBuilderTest class? https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:412: GraphGeneratorHelper helper(isolate(), zone(), code_snippet); I understand that it is tempting to generate the bytecode from a source string. But this is no longer a real unit test, because it requires the entire parser to run. How would you feel about turning these into a cctest eventually (not in this CL)?
https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:129: i::FLAG_ignition = true; Ouch! Changing global flags in unit tests is a no-go. We want to be able to eventually run all of them in a single process. This really should be turned into a cctest!
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:47: #define REPEAT_127(SEP, ...) \ Do we really need/want these macros here? That looks like we should be able to get away without them. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:129: i::FLAG_ignition = true; Don't change the flags in a unittest. This is not going to work as expected. The idea for unittests is that you can run the binary itself, which means that unit tests have to be unit tests and must not mutate the global state. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:136: FlagList::SetFlagsFromString(ignition_filter.start(), Same here. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:139: isolate_->interpreter()->Initialize(); And here. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:447: REPEAT_127(SPACE, " b = p1.val_prev; ") Using strings here is not a good idea, because then your unit tests depend on the parser (and a lot of other machinery) and are essentially not unit tests, but system tests. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:468: const char code_snippet[] = "function f(p1) {'use strict'; var b;" Same here.
https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:47: #define REPEAT_127(SEP, ...) \ On 2015/11/12 13:45:02, Benedikt Meurer wrote: > Do we really need/want these macros here? That looks like we should be able to > get away without them. This is to force the code to emit a Wide bytecode variant https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:129: i::FLAG_ignition = true; On 2015/11/12 13:45:02, Benedikt Meurer wrote: > Don't change the flags in a unittest. This is not going to work as expected. The > idea for unittests is that you can run the binary itself, which means that unit > tests have to be unit tests and must not mutate the global state. Yeah sorry, missed this. I'll move this to be a cctest.
I removed the bytecode generation from strings and reverted it back to building the bytecode array. I could not move it a cctest because we use Matcher<Node*> which depend on gmock. Could you please review the changes. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:47: #define REPEAT_127(SEP, ...) \ On 2015/11/12 13:48:56, rmcilroy wrote: > On 2015/11/12 13:45:02, Benedikt Meurer wrote: > > Do we really need/want these macros here? That looks like we should be able to > > get away without them. > > This is to force the code to emit a Wide bytecode variant With the new changes, they are not required. So removed them. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:129: i::FLAG_ignition = true; On 2015/11/12 13:48:56, rmcilroy wrote: > On 2015/11/12 13:45:02, Benedikt Meurer wrote: > > Don't change the flags in a unittest. This is not going to work as expected. > The > > idea for unittests is that you can run the binary itself, which means that > unit > > tests have to be unit tests and must not mutate the global state. > > Yeah sorry, missed this. I'll move this to be a cctest. Since we use Matcher<Node*>, we retained this in unittest and changed it to build the bytecode array. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:136: FlagList::SetFlagsFromString(ignition_filter.start(), On 2015/11/12 13:45:02, Benedikt Meurer wrote: > Same here. Done. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:139: isolate_->interpreter()->Initialize(); On 2015/11/12 13:45:02, Benedikt Meurer wrote: > And here. Done. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:447: REPEAT_127(SPACE, " b = p1.val_prev; ") On 2015/11/12 13:45:02, Benedikt Meurer wrote: > Using strings here is not a good idea, because then your unit tests depend on > the parser (and a lot of other machinery) and are essentially not unit tests, > but system tests. Done. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:468: const char code_snippet[] = "function f(p1) {'use strict'; var b;" On 2015/11/12 13:45:02, Benedikt Meurer wrote: > Same here. Done.
LGTM from my end. https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right): https://codereview.chromium.org/1419373007/diff/120001/test/unittests/compile... test/unittests/compiler/bytecode-graph-builder-unittest.cc:129: i::FLAG_ignition = true; On 2015/11/13 09:42:56, mythria wrote: > On 2015/11/12 13:48:56, rmcilroy wrote: > > On 2015/11/12 13:45:02, Benedikt Meurer wrote: > > > Don't change the flags in a unittest. This is not going to work as expected. > > The > > > idea for unittests is that you can run the binary itself, which means that > > unit > > > tests have to be unit tests and must not mutate the global state. > > > > Yeah sorry, missed this. I'll move this to be a cctest. > > Since we use Matcher<Node*>, we retained this in unittest and changed it to > build the bytecode array. Acknowledged. Even better. Awesome!
LGTM, thanks.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1419373007/#ps160001 (title: "Modified bytecode-graph-builder-unittest to build bytecode array instead of generating it.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1419373007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1419373007/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2acc2bc2a1a1dfc9d681e028a44f3657a99ebf1a Cr-Commit-Position: refs/heads/master@{#32026} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
