Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159303002:
[turbofan] Improve the store-store elimination. (Closed)
Description[turbofan] Improve the store-store elimination.
It can now deal with multiple objects at the same time (but no
aliasing), and it propagates store information upwards across effect
chain splits.
R=jarin
BUG=
Committed: https://crrev.com/5caabdd5ccf549bec8553a76c6128cee78a3af91
Cr-Commit-Position: refs/heads/master@{#38509}
Patch Set 1 #Patch Set 2 : Silence silly compiler warnings. #
Total comments: 18
Patch Set 3 : Incorporate feedback. #Patch Set 4 : Fix a compile error in release mode and a compile error for MSVC. #
Total comments: 1
Patch Set 5 : Fix dchecked invariant: Start is the only node that may have effect use but no input #Patch Set 6 : Cut down on documentation and tracing. There is now only a single trace level. #
Total comments: 31
Patch Set 7 : Address feedback on reviewed part. #Patch Set 8 : Determined -> visited. #
Total comments: 18
Patch Set 9 : Address 2 out of 6 feedbacksies. #Patch Set 10 : Iterate differently over control inputs. Cache VisitedEmpty. #Patch Set 11 : Merge with master. #Patch Set 12 : Fix merge conflict #
Messages
Total messages: 58 (42 generated)
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
First batch of comments. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/2159303002/diff/20001/src/compiler/pipeline.c... src/compiler/pipeline.cc:1538: Run<CommonOperatorReducerPhase>(); Just curious - why is this necessary? (I assume it is for some dead code elimination - we should probably rename to DeadCodeEliminationPhase, then.) https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:29: } while (false) This feels a bit over-engineered - normally, you would be the only one staring at the diagnostic output; one general TRACE (no levels) should be enough. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:105: // changes to an UnobservablesSet allocate in the temp_zone. This blurb should go into the UnobservablesSet class. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:130: StoreOffset to_offset(int offset) { Function names should be CamelCase. (Here and below.) https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:330: return (node->op()->properties() & mask) == mask || node->op()->HasProperty(Operator::kNoRead | Operator::kNoDeopt | Operator::kNoThrow) ... https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... File src/compiler/store-store-elimination.h (right): https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:20: }; All the stuff below should go to the .cc file since it is local to the pass. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:26: struct StoreObservation { The name is a bit confusing because it seems to describe a field that is not observed. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:47: ~UnobservablesSet() {} Why is the destructor here? https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:78: virtual Reduction Reduce(Node* node); I have difficulty understanding why this is a reducer - reducers are forward passes, store elimination is backwards.
See below. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/2159303002/diff/20001/src/compiler/pipeline.c... src/compiler/pipeline.cc:1538: Run<CommonOperatorReducerPhase>(); On 2016/07/20 11:43:33, Jarin wrote: > Just curious - why is this necessary? (I assume it is for some dead code > elimination - we should probably rename to DeadCodeEliminationPhase, then.) To eliminate dead conditional deopts. This is done by CommonOperatorReducer, which needs to run concurrently with DeadCodeElimination if I understand correctly. Added a comment to the CL. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:29: } while (false) On 2016/07/20 11:43:33, Jarin wrote: > This feels a bit over-engineered - normally, you would be the only one staring > at the diagnostic output; one general TRACE (no levels) should be enough. Because trace level 1 is ~37% as big as trace level 5. In level 1 it's very easy to spot how often we revisit a chain, and it's easy to see what nodes observe everything / observe nothing / are observable/unobservable StoreFields. I agree that there currently are a lot of TRACE2s left though; should I try to cut them down? https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:105: // changes to an UnobservablesSet allocate in the temp_zone. On 2016/07/20 11:43:33, Jarin wrote: > This blurb should go into the UnobservablesSet class. Done. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:130: StoreOffset to_offset(int offset) { On 2016/07/20 11:43:33, Jarin wrote: > Function names should be CamelCase. (Here and below.) Yeah, you're right. I was thinking of the "simple function" exception, but that doesn't really apply here any more. Fixed, with the exception of rep_size_of. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:330: return (node->op()->properties() & mask) == mask || On 2016/07/20 11:43:33, Jarin wrote: > node->op()->HasProperty(Operator::kNoRead | Operator::kNoDeopt | > Operator::kNoThrow) ... That doesn't compile, because Operator::kNoRead | ... is of type Operator::Properties, not Operator::Property. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... File src/compiler/store-store-elimination.h (right): https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:20: }; On 2016/07/20 11:43:33, Jarin wrote: > All the stuff below should go to the .cc file since it is local to the pass. Done. I put it in an anonymous namespace. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:26: struct StoreObservation { On 2016/07/20 11:43:33, Jarin wrote: > The name is a bit confusing because it seems to describe a field that is not > observed. Changed to UnobservableStore. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:47: ~UnobservablesSet() {} On 2016/07/20 11:43:33, Jarin wrote: > Why is the destructor here? Because I thought that the implicitly generated destructor would destroy the pointer fields. Instead it's an empty method. Removed. https://codereview.chromium.org/2159303002/diff/20001/src/compiler/store-stor... src/compiler/store-store-elimination.h:78: virtual Reduction Reduce(Node* node); On 2016/07/20 11:43:33, Jarin wrote: > I have difficulty understanding why this is a reducer - reducers are forward > passes, store elimination is backwards. Changed as discussed.
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by bgeron@google.com
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
https://codereview.chromium.org/2159303002/diff/60001/src/compiler/store-stor... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/60001/src/compiler/store-stor... src/compiler/store-store-elimination.cc:356: DCHECK(isEffectful); Tests currently fail on this. Checking this only for non-Start nodes might make the check pass.
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Cut down on documentation and tracing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2159303002/diff/100001/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/2159303002/diff/100001/src/compiler/pipeline.... src/compiler/pipeline.cc:1039: struct CommonOperatorReducerPhase { Rename to DeadCodeEliminationPhase (because that is what it is doing). https://codereview.chromium.org/2159303002/diff/100001/src/compiler/pipeline.... src/compiler/pipeline.cc:1059: GraphTrimmer(temp_zone, data->graph()).TrimGraph(); Could you call the trimming on the roots, too? (As we do right below, in the LoadEliminationPhase.) Otherwise you are not guaranteed the graph will be really trimmed. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:138: class StoreStoreFinder final { Maybe rename to RedundantStoreFinder? (Or UnobservableStoreFinder?) https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:196: // more than one Leave out the comment (or make into a sentence with first capital letter and a dot at the end.) https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:213: } else { How about leaving out the else branches and just have unconditional 'return nullptr;' at the end of the function? https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:221: size_t rep_size_of(MachineRepresentation rep) { CamelCase, please. (Here and below.) https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:254: in_revisit_.at(next->id()) = false; Do not use the 'at' method - it can throw exceptions and we do not really want that. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:267: } Niiiice, I love this check; will use this elsewhere. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:303: if (endsEffectChain && EffectUseCount(node) >= 1 && The if is here just for the DCHECK, please fold the condition into the DCHECK, so that we do not compute it needlessly in release mode. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:317: // CanObserveNothing), but there are some opcodes we treat specially. This comment basically just states what is obvious from the code below, so maybe remove it? https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:325: bool presentInSet = uses.Contains(observation); presentInSet => isNotObservable? https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:372: return uses.RemoveSameOffset(offset, temp_zone()); In future, we could be a bit smarter here and only remove abstract locations that may alias with the load. In particular, if this load from an allocation node and the abstract location is from a different allocation, then we do not have to invalidate that location. (Not actionable now.) https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:401: bool StoreStoreFinder::CanObserveNothing(Node* node) { CannotObserveStoreField? https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:414: if (opcode == IrOpcode::kLoad) { Benedikt says kLoad should never alias a field, so maybe this is not needed. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:439: if (IsEligibleNode(node)) { I do not understand what is the advantage of walking effect chains in the VisitEligibleNode? Would not it be simpler just to take one node at a time and propagate the observation information through the queue and the per-node observation sets?
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. See below. Also, don't forgot to review the part you hadn't reviewed yet at the bottom (which got ~40% shorter because I found dead/useless code). https://codereview.chromium.org/2159303002/diff/100001/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/2159303002/diff/100001/src/compiler/pipeline.... src/compiler/pipeline.cc:1039: struct CommonOperatorReducerPhase { On 2016/07/25 09:52:13, Jarin wrote: > Rename to DeadCodeEliminationPhase (because that is what it is doing). Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/pipeline.... src/compiler/pipeline.cc:1059: GraphTrimmer(temp_zone, data->graph()).TrimGraph(); On 2016/07/25 09:52:13, Jarin wrote: > Could you call the trimming on the roots, too? (As we do right below, in the > LoadEliminationPhase.) Otherwise you are not guaranteed the graph will be really > trimmed. Done. Actually, I think the graph would be trimmed a bit too far :D https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:138: class StoreStoreFinder final { On 2016/07/25 09:52:13, Jarin wrote: > Maybe rename to RedundantStoreFinder? (Or UnobservableStoreFinder?) Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:196: // more than one On 2016/07/25 09:52:13, Jarin wrote: > Leave out the comment (or make into a sentence with first capital letter and a > dot at the end.) Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:213: } else { On 2016/07/25 09:52:13, Jarin wrote: > How about leaving out the else branches and just have unconditional 'return > nullptr;' at the end of the function? Nice spot. Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:221: size_t rep_size_of(MachineRepresentation rep) { On 2016/07/25 09:52:13, Jarin wrote: > CamelCase, please. (Here and below.) Done. I left `unobservable_for_id` as is, because the style guide seems to be okay with that. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:254: in_revisit_.at(next->id()) = false; On 2016/07/25 09:52:13, Jarin wrote: > Do not use the 'at' method - it can throw exceptions and we do not really want > that. Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:267: } On 2016/07/25 09:52:13, Jarin wrote: > Niiiice, I love this check; will use this elsewhere. We should put it in src/base/logging.h :D https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:303: if (endsEffectChain && EffectUseCount(node) >= 1 && On 2016/07/25 09:52:13, Jarin wrote: > The if is here just for the DCHECK, please fold the condition into the DCHECK, > so that we do not compute it needlessly in release mode. Method disappeared during refactoring. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:317: // CanObserveNothing), but there are some opcodes we treat specially. On 2016/07/25 09:52:13, Jarin wrote: > This comment basically just states what is obvious from the code below, so maybe > remove it? Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:325: bool presentInSet = uses.Contains(observation); On 2016/07/25 09:52:13, Jarin wrote: > presentInSet => isNotObservable? Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:372: return uses.RemoveSameOffset(offset, temp_zone()); On 2016/07/25 09:52:13, Jarin wrote: > In future, we could be a bit smarter here and only remove abstract locations > that may alias with the load. In particular, if this load from an allocation > node and the abstract location is from a different allocation, then we do not > have to invalidate that location. > > (Not actionable now.) Ack. Reminds me.. for the load elimination, I encountered multiple references to the same allocation but some through a CheckIf; I implemented some normalisation procedure for this in commit 37330 (see link below). I didn't encounter such indirections yet for StoreField, but if we do, it might be worth porting over this normalisation. https://github.com/v8/v8/commit/f416886358c0aeaa5b4d1e83d9c7d6bb8b0e7f2f https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:401: bool StoreStoreFinder::CanObserveNothing(Node* node) { On 2016/07/25 09:52:13, Jarin wrote: > CannotObserveStoreField? Done. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:414: if (opcode == IrOpcode::kLoad) { On 2016/07/25 09:52:13, Jarin wrote: > Benedikt says kLoad should never alias a field, so maybe this is not needed. Well, if I make it so that Load cannot observe anything, the tests do pass! Funny. I thought I'd seen Load with computed pointer arguments, but I guess at the moment we don't happen to use that to access fields. I've added it to the new patchset, but I can easily roll that back still. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:439: if (IsEligibleNode(node)) { On 2016/07/25 09:52:13, Jarin wrote: > I do not understand what is the advantage of walking effect chains in the > VisitEligibleNode? Would not it be simpler just to take one node at a time and > propagate the observation information through the queue and the per-node > observation sets? Good idea; this simplified the file significantly. I couldn't measure a slowdown in Octane. Changed the tracing to still make sense. https://codereview.chromium.org/2159303002/diff/100001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:480: DCHECK(before_set.IsEmpty()); Oops!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost there, I like it! https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:186: return ((size_t)1) << ElementSizeLog2Of(rep); ((size_t)1) -> 1u https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:381: for (Edge edge : node->input_edges()) { You can also say: for (int i = 0; i < node->op()->ControlInputCount(); i++) { Node* control_input = NodeProperties::GetControlInput(node, i); if (!HasBeenVisited(control_input)) { MarkForRevisit(control_input); } } https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:397: UnobservablesSet::VisitedEmpty(temp_zone()); Could this be moved to the control node case? (After line 385.) https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:422: if (!CanObserveAnything(input) || !HasBeenVisited(input)) { Is this condition necessary? If the node can observe anything and has been visited before, then the non-observation set should be empty, so it should never even get here (because cur_set_changed should be false). No? https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:492: // can probably be optimized to use a global singleton. Global singletons are forbidden, but you might want to cache this set in the finder class. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:506: } else if (other.set() == nullptr) { This case is unreachable, no? (It is already handled above.)
https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:186: return ((size_t)1) << ElementSizeLog2Of(rep); On 2016/08/01 08:25:43, Jarin wrote: > ((size_t)1) -> 1u Hm, I think that would be an unsigned int, which is often smaller than size_t. I'm considering changing it just to int, to simplify everything. It's not as if we can reasonably expect any field to be bigger than 2^20 anyway. (Included in CL now.) What do you think? https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:381: for (Edge edge : node->input_edges()) { On 2016/08/01 08:25:43, Jarin wrote: > You can also say: > > for (int i = 0; i < node->op()->ControlInputCount(); i++) { > Node* control_input = NodeProperties::GetControlInput(node, i); > if (!HasBeenVisited(control_input)) { > MarkForRevisit(control_input); > } > } Is the speed the reason that that fragment might be better? I actually feel both of these are 'wrong', and it should be something like for (Node* control_input : node->control_inputs()) { if (!HasBeenVisited(control_input)) { MarkForRevisit(control_input); } } I think this should be as fast as your proposal, and it's more readable than either proposal, and this pattern (iterate over value/effect/control inputs) occurs really often, in my mind. (As a separate thing, I also fail to see the importance of having node properties in a separate file/class: there are many trivial functions there that would be much nicer as methods on Node or Edge.) https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:397: UnobservablesSet::VisitedEmpty(temp_zone()); On 2016/08/01 08:25:43, Jarin wrote: > Could this be moved to the control node case? (After line 385.) Not that I know of. In some cases, VisitEffectfulNode behaves differently on unvisited nodes than on visited nodes. If it finds a visited node computes that its UnobservablesSet is still empty, then it does not revisit its effect inputs. If it finds an unvisited node and computes an empty UnobservablesSet, it will still (re)visit the effect inputs, which are probably also unvisited. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:422: if (!CanObserveAnything(input) || !HasBeenVisited(input)) { On 2016/08/01 08:25:43, Jarin wrote: > Is this condition necessary? If the node can observe anything and has been > visited before, then the non-observation set should be empty, so it should never > even get here (because cur_set_changed should be false). No? No. :) Well, it's not strictly necessary because the algorithm will be correct nonetheless, but it saves some revisits in the case of a Call that can deopt. We visit the whole control graph, so we might visit the IfSuccess of a Call, and then the Call; the effect outputs of the Call have not been visited yet. Some time later, the effect outputs are visited, and the Call might be re-visited from both effect outputs if it weren't for this check, even though the Call observes everything. The trace output is significantly shorter with this check, so it's obvious that the algorithm doesn't too many superfluous revisits. However, I suppose the question is whether this check makes things slower or faster. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:492: // can probably be optimized to use a global singleton. On 2016/08/01 08:25:43, Jarin wrote: > Global singletons are forbidden, but you might want to cache this set in the > finder class. I know, it's silly right? But if we memoize a VisitedEmpty, then that should probably not be in the temp_zone. (Unless we want to get exciting memory corruption if we'll ever try to execute this pass twice.) I suppose maybe we should allocate it in the global zone. How would I find that? Can I just use malloc? https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:506: } else if (other.set() == nullptr) { On 2016/08/01 08:25:43, Jarin wrote: > This case is unreachable, no? (It is already handled above.) Just checking if you were paying attention, of course! Fixed and improved.
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:381: for (Edge edge : node->input_edges()) { On 2016/08/04 12:31:02, bgeron wrote: > On 2016/08/01 08:25:43, Jarin wrote: > > You can also say: > > > > for (int i = 0; i < node->op()->ControlInputCount(); i++) { > > Node* control_input = NodeProperties::GetControlInput(node, i); > > if (!HasBeenVisited(control_input)) { > > MarkForRevisit(control_input); > > } > > } > > Is the speed the reason that that fragment might be better? Yes. Also consistency with the rest of the codebase. > I actually feel both of these are 'wrong', and it should be something like > > for (Node* control_input : node->control_inputs()) { > if (!HasBeenVisited(control_input)) { > MarkForRevisit(control_input); > } > } > > I think this should be as fast as your proposal, and it's more readable than > either proposal, and this pattern (iterate over value/effect/control inputs) > occurs really often, in my mind. > > (As a separate thing, I also fail to see the importance of having node > properties in a separate file/class: there are many trivial functions there that > would be much nicer as methods on Node or Edge.) I tend to agree. The idea here was that the Node class does not really know anything about the control/effect/value distinction. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:397: UnobservablesSet::VisitedEmpty(temp_zone()); On 2016/08/04 12:31:02, bgeron wrote: > On 2016/08/01 08:25:43, Jarin wrote: > > Could this be moved to the control node case? (After line 385.) > > Not that I know of. In some cases, VisitEffectfulNode behaves differently on > unvisited nodes than on visited nodes. If it finds a visited node computes that > its UnobservablesSet is still empty, then it does not revisit its effect inputs. > If it finds an unvisited node and computes an empty UnobservablesSet, it will > still (re)visit the effect inputs, which are probably also unvisited. Acknowledged. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:422: if (!CanObserveAnything(input) || !HasBeenVisited(input)) { On 2016/08/04 12:31:02, bgeron wrote: > On 2016/08/01 08:25:43, Jarin wrote: > > Is this condition necessary? If the node can observe anything and has been > > visited before, then the non-observation set should be empty, so it should > never > > even get here (because cur_set_changed should be false). No? > > No. :) Well, it's not strictly necessary because the algorithm will be correct > nonetheless, but it saves some revisits in the case of a Call that can deopt. We > visit the whole control graph, so we might visit the IfSuccess of a Call, and > then the Call; the effect outputs of the Call have not been visited yet. Some > time later, the effect outputs are visited, and the Call might be re-visited > from both effect outputs if it weren't for this check, even though the Call > observes everything. > > The trace output is significantly shorter with this check, so it's obvious that > the algorithm doesn't too many superfluous revisits. However, I suppose the > question is whether this check makes things slower or faster. Yeah, I was confused. Thanks for the explanation. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:492: // can probably be optimized to use a global singleton. On 2016/08/04 12:31:02, bgeron wrote: > On 2016/08/01 08:25:43, Jarin wrote: > > Global singletons are forbidden, but you might want to cache this set in the > > finder class. > > I know, it's silly right? But if we memoize a VisitedEmpty, then that should > probably not be in the temp_zone. (Unless we want to get exciting memory > corruption if we'll ever try to execute this pass twice.) > > I suppose maybe we should allocate it in the global zone. How would I find that? > Can I just use malloc? > graph()->zone()
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... File src/compiler/store-store-elimination.cc (right): https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:381: for (Edge edge : node->input_edges()) { On 2016/08/05 13:49:49, Jarin wrote: > On 2016/08/04 12:31:02, bgeron wrote: > > On 2016/08/01 08:25:43, Jarin wrote: > > > You can also say: > > > > > > for (int i = 0; i < node->op()->ControlInputCount(); i++) { > > > Node* control_input = NodeProperties::GetControlInput(node, i); > > > if (!HasBeenVisited(control_input)) { > > > MarkForRevisit(control_input); > > > } > > > } > > > > Is the speed the reason that that fragment might be better? > > Yes. Also consistency with the rest of the codebase. > > > I actually feel both of these are 'wrong', and it should be something like > > > > for (Node* control_input : node->control_inputs()) { > > if (!HasBeenVisited(control_input)) { > > MarkForRevisit(control_input); > > } > > } > > > > I think this should be as fast as your proposal, and it's more readable than > > either proposal, and this pattern (iterate over value/effect/control inputs) > > occurs really often, in my mind. > > > > (As a separate thing, I also fail to see the importance of having node > > properties in a separate file/class: there are many trivial functions there > that > > would be much nicer as methods on Node or Edge.) > > I tend to agree. The idea here was that the Node class does not really know > anything about the control/effect/value distinction. > Changed to your suggestion; we can refactor all the things in a separate CL. https://codereview.chromium.org/2159303002/diff/130001/src/compiler/store-sto... src/compiler/store-store-elimination.cc:492: // can probably be optimized to use a global singleton. On 2016/08/05 13:49:49, Jarin wrote: > On 2016/08/04 12:31:02, bgeron wrote: > > On 2016/08/01 08:25:43, Jarin wrote: > > > Global singletons are forbidden, but you might want to cache this set in the > > > finder class. > > > > I know, it's silly right? But if we memoize a VisitedEmpty, then that should > > probably not be in the temp_zone. (Unless we want to get exciting memory > > corruption if we'll ever try to execute this pass twice.) > > > > I suppose maybe we should allocate it in the global zone. How would I find > that? > > Can I just use malloc? > > > > graph()->zone() Done, as discussed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10683) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10666) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10625) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21446)
The CQ bit was checked by bgeron@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10669)
The CQ bit was checked by bgeron@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2159303002/#ps210001 (title: "Fix merge conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Improve the store-store elimination. It can now deal with multiple objects at the same time (but no aliasing), and it propagates store information upwards across effect chain splits. R=jarin BUG= ========== to ========== [turbofan] Improve the store-store elimination. It can now deal with multiple objects at the same time (but no aliasing), and it propagates store information upwards across effect chain splits. R=jarin BUG= Committed: https://crrev.com/5caabdd5ccf549bec8553a76c6128cee78a3af91 Cr-Commit-Position: refs/heads/master@{#38509} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5caabdd5ccf549bec8553a76c6128cee78a3af91 Cr-Commit-Position: refs/heads/master@{#38509}
Message was sent while issue was closed.
On 2016/08/09 20:00:48, commit-bot: I haz the power wrote: > Patchset 12 (id:??) landed as > https://crrev.com/5caabdd5ccf549bec8553a76c6128cee78a3af91 > Cr-Commit-Position: refs/heads/master@{#38509} In src/compiler/store-store-elimination.cc: class RedundantStoreFinder defined a virtual member function, virtual void Visit(Node* node), but didn't defined a virtual deconstructor. The GCC compiler will complain: error: ‘class v8::internal::compiler::{anonymous}::RedundantStoreFinder’ has virtual functions and accessible non-virtual destructor [-Werror=non-virtual-dtor] Would you please fix this issue?
Message was sent while issue was closed.
On 2016/08/10 02:37:14, zhengxing.li wrote: > On 2016/08/09 20:00:48, commit-bot: I haz the power wrote: > > Patchset 12 (id:??) landed as > > https://crrev.com/5caabdd5ccf549bec8553a76c6128cee78a3af91 > > Cr-Commit-Position: refs/heads/master@{#38509} > > In src/compiler/store-store-elimination.cc: class RedundantStoreFinder defined a > virtual member function, virtual void Visit(Node* node), but didn't defined a > virtual deconstructor. > > The GCC compiler will complain: > error: ‘class v8::internal::compiler::{anonymous}::RedundantStoreFinder’ has > virtual functions and accessible non-virtual destructor > [-Werror=non-virtual-dtor] > > Would you please fix this issue? Should be fixed now (https://codereview.chromium.org/2234703002/). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
