Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18596005:
Allocation folding integrated into the GVN phase. (Closed)
|
Created:
7 years, 5 months ago by Hannes Payer (out of office) Modified:
7 years, 5 months ago CC:
v8-dev Visibility:
Public. |
DescriptionAllocation folding integrated into the GVN phase.
BUG=
R=mstarzinger@chromium.org, titzer@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=15624
Patch Set 1 #Patch Set 2 : #
Total comments: 27
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3208: if (FLAG_use_allocation_folding) { if (!FLAG_...) return; if (!...) return; Can remove two levels of nesting here. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3230: HInstruction* dominator_instr = HInstruction::cast(dominator); You already have dominator as dominator_allocate_instr. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3240: HConstant* filler_map = block->graph()->GetConstantFreeSpaceMap(); Don't make this a global constant in the graph. That increases its live range, thus register pressure and spilling. It's fine to create this constant at each allocation site, IMO. Maybe if you have multiple folded allocations you can insert this constant after the intial HAllocate and reuse the same one, but making it global is probably not worth it. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { Please don't add more flags to this constructor; you should probably set these flags in your special insertion routine. I'm not really sure why you are doing this anyway...since I don't think the instructions are GVN'd after that....
First round, want to finish this round early as it turns out I am writing the same comments a Ben. :) https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc File src/hydrogen-gvn.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcod... src/hydrogen-gvn.cc:779: for (HInstructionIterator it(block); !it.Done(); it.Advance()) { Switching to the HInstructionIterator will break for cases where the instruction is unlinked from the graph. See the following comment about that. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcod... src/hydrogen-gvn.cc:799: if (instr->IsLinked() && !flags.IsEmpty()) { Instead of doing the IsLinked() checks here, could we just to a "continue" after the side-effect dominator notification? Of course that require us to make HInstructionIterator resilient against concurrent modification (i.e. remember the "next_" field in the iterator). https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcod... src/hydrogen-gvn.cc:807: if (instr->IsLinked() && instr->CheckFlag(HValue::kUseGVN)) { Likewise. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:1658: HValue* dominator) { nit: Indentation if off. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3206: HValue* dominator) { nit: Indentation is off. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3208: if (FLAG_use_allocation_folding) { Can we do early returns here instead of indenting the whole block, that makes this whole method easier to read. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3240: HConstant* filler_map = block->graph()->GetConstantFreeSpaceMap(); On 2013/07/08 12:40:14, titzer wrote: > Don't make this a global constant in the graph. That increases its live range, > thus register pressure and spilling. It's fine to create this constant at each > allocation site, IMO. Maybe if you have multiple folded allocations you can > insert this constant after the intial HAllocate and reuse the same one, but > making it global is probably not worth it. I don't think this is an issue as the HConstant will not get a live range. It will be embedded as a constant at every use-site. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { Can we add a TODO here that this argument should go away once we no longer need to do initialization of folded allocation segments?
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc File src/hydrogen-gvn.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcod... src/hydrogen-gvn.cc:779: for (HInstructionIterator it(block); !it.Done(); it.Advance()) { On 2013/07/08 12:44:06, Michael Starzinger wrote: > Switching to the HInstructionIterator will break for cases where the instruction > is unlinked from the graph. See the following comment about that. Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcod... src/hydrogen-gvn.cc:799: if (instr->IsLinked() && !flags.IsEmpty()) { On 2013/07/08 12:44:06, Michael Starzinger wrote: > Instead of doing the IsLinked() checks here, could we just to a "continue" after > the side-effect dominator notification? Of course that require us to make > HInstructionIterator resilient against concurrent modification (i.e. remember > the "next_" field in the iterator). Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcod... src/hydrogen-gvn.cc:807: if (instr->IsLinked() && instr->CheckFlag(HValue::kUseGVN)) { On 2013/07/08 12:44:06, Michael Starzinger wrote: > Likewise. Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:1658: HValue* dominator) { On 2013/07/08 12:44:06, Michael Starzinger wrote: > nit: Indentation if off. Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3206: HValue* dominator) { On 2013/07/08 12:44:06, Michael Starzinger wrote: > nit: Indentation is off. Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3208: if (FLAG_use_allocation_folding) { On 2013/07/08 12:40:14, titzer wrote: > if (!FLAG_...) return; > if (!...) return; > > Can remove two levels of nesting here. Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3230: HInstruction* dominator_instr = HInstruction::cast(dominator); On 2013/07/08 12:40:14, titzer wrote: > You already have dominator as dominator_allocate_instr. Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3240: HConstant* filler_map = block->graph()->GetConstantFreeSpaceMap(); On 2013/07/08 12:40:14, titzer wrote: > Don't make this a global constant in the graph. That increases its live range, > thus register pressure and spilling. It's fine to create this constant at each > allocation site, IMO. Maybe if you have multiple folded allocations you can > insert this constant after the intial HAllocate and reuse the same one, but > making it global is probably not worth it. OK. I am leaving this code since it will disappear anyway after fixing the new space iterator. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { On 2013/07/08 12:44:06, Michael Starzinger wrote: > Can we add a TODO here that this argument should go away once we no longer need > to do initialization of folded allocation segments? Done. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { On 2013/07/08 12:40:14, titzer wrote: > Please don't add more flags to this constructor; you should probably set these > flags in your special insertion routine. I'm not really sure why you are doing > this anyway...since I don't think the instructions are GVN'd after that.... This is needed otherwise the HGraph Verifier is not happy. But I will add a TODO as Michael suggested.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3240: HConstant* filler_map = block->graph()->GetConstantFreeSpaceMap(); On 2013/07/08 13:43:48, Hannes Payer wrote: > On 2013/07/08 12:40:14, titzer wrote: > > Don't make this a global constant in the graph. That increases its live range, > > thus register pressure and spilling. It's fine to create this constant at each > > allocation site, IMO. Maybe if you have multiple folded allocations you can > > insert this constant after the intial HAllocate and reuse the same one, but > > making it global is probably not worth it. > OK. > > I am leaving this code since it will disappear anyway after fixing the new space > iterator. Why? If it is not so important, then please do not pollute HGraph with another global constant. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { On 2013/07/08 13:43:48, Hannes Payer wrote: > On 2013/07/08 12:40:14, titzer wrote: > > Please don't add more flags to this constructor; you should probably set these > > flags in your special insertion routine. I'm not really sure why you are doing > > this anyway...since I don't think the instructions are GVN'd after that.... > > This is needed otherwise the HGraph Verifier is not happy. But I will add a TODO > as Michael suggested. In either case, you should set the flag where you are creating the instruction, not in the constructor.
Almost looking good. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.cc:3240: HConstant* filler_map = block->graph()->GetConstantFreeSpaceMap(); On 2013/07/08 13:49:38, titzer wrote: > On 2013/07/08 13:43:48, Hannes Payer wrote: > > On 2013/07/08 12:40:14, titzer wrote: > > > Don't make this a global constant in the graph. That increases its live > range, > > > thus register pressure and spilling. It's fine to create this constant at > each > > > allocation site, IMO. Maybe if you have multiple folded allocations you can > > > insert this constant after the intial HAllocate and reuse the same one, but > > > making it global is probably not worth it. > > OK. > > > > I am leaving this code since it will disappear anyway after fixing the new > space > > iterator. > > Why? If it is not so important, then please do not pollute HGraph with another > global constant. I kind of see Ben's point here. If it is possible to come up with a more local temporary workaround (aka. hack) that would definitely be preferable. Especially since one of Danno's upcoming changes will actually track live-ranges for constants with more than one use (which renders my original comment moot). So +1 on this comments as well. https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { On 2013/07/08 13:49:38, titzer wrote: > On 2013/07/08 13:43:48, Hannes Payer wrote: > > On 2013/07/08 12:40:14, titzer wrote: > > > Please don't add more flags to this constructor; you should probably set > these > > > flags in your special insertion routine. I'm not really sure why you are > doing > > > this anyway...since I don't think the instructions are GVN'd after that.... > > > > This is needed otherwise the HGraph Verifier is not happy. But I will add a > TODO > > as Michael suggested. > > In either case, you should set the flag where you are creating the instruction, > not in the constructor. +1 on Ben's last comment. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen-gvn.cc File src/hydrogen-gvn.cc (right): https://codereview.chromium.org/18596005/diff/10001/src/hydrogen-gvn.cc#newco... src/hydrogen-gvn.cc:800: continue; nit: The whole condition should fit into one line. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h#newcode237 src/hydrogen.h:237: : instr_(block->first()), next_(NULL) { nit: Use a ternary operator, easier to read. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h#newcode247 src/hydrogen.h:247: if (!Done()) { nit: Use a ternary operator, easier to read.
https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.cc#newcode666 src/hydrogen.cc:666: HConstant* HGraph::GetConstantFreeSpaceMap() { Please don't introduce another constant at the top of the graph. They artificially extend live ranges, and given some changes I have planned for better live range handling of constants, they are counter-productive.
LGTM (after comments are addressed). As discussed offline: I am fine with the current state, as long as Ben's and Danno's comments are addressed and GVN isn't changed anymore.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.... src/hydrogen-instructions.h:5811: if (no_observable_side_effects) { On 2013/07/08 13:49:38, titzer wrote: > On 2013/07/08 13:43:48, Hannes Payer wrote: > > On 2013/07/08 12:40:14, titzer wrote: > > > Please don't add more flags to this constructor; you should probably set > these > > > flags in your special insertion routine. I'm not really sure why you are > doing > > > this anyway...since I don't think the instructions are GVN'd after that.... > > > > This is needed otherwise the HGraph Verifier is not happy. But I will add a > TODO > > as Michael suggested. > > In either case, you should set the flag where you are creating the instruction, > not in the constructor. Done. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen-gvn.cc File src/hydrogen-gvn.cc (right): https://codereview.chromium.org/18596005/diff/10001/src/hydrogen-gvn.cc#newco... src/hydrogen-gvn.cc:800: continue; On 2013/07/08 14:02:43, Michael Starzinger wrote: > nit: The whole condition should fit into one line. Done. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.cc#newcode666 src/hydrogen.cc:666: HConstant* HGraph::GetConstantFreeSpaceMap() { On 2013/07/08 14:35:39, danno wrote: > Please don't introduce another constant at the top of the graph. They > artificially extend live ranges, and given some changes I have planned for > better live range handling of constants, they are counter-productive. Done. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h#newcode237 src/hydrogen.h:237: : instr_(block->first()), next_(NULL) { On 2013/07/08 14:02:43, Michael Starzinger wrote: > nit: Use a ternary operator, easier to read. Done. https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h#newcode247 src/hydrogen.h:247: if (!Done()) { On 2013/07/08 14:02:43, Michael Starzinger wrote: > nit: Use a ternary operator, easier to read. Done.
lgtm https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3043: HType HInnerAllocatedObject::CalculateInferredType() { Can put this in the header. https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3224: Add comment to the effect of "try to fold allocations together with their dominating allocations" https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3246: Representation::None(), I think you want Representation::Tagged() https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.h:4982: Whitespace changes. https://codereview.chromium.org/18596005/diff/22001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/18596005/diff/22001/src/hydrogen.h#newcode241 src/hydrogen.h:241: bool Done() { return instr_ == NULL; } While you're here, would you mind adding "inline" and "const" keywords?
https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3043: HType HInnerAllocatedObject::CalculateInferredType() { On 2013/07/09 08:34:39, titzer wrote: > Can put this in the header. Done. https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3224: On 2013/07/09 08:34:39, titzer wrote: > Add comment to the effect of "try to fold allocations together with their > dominating allocations" Done. https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3246: Representation::None(), On 2013/07/09 08:34:39, titzer wrote: > I think you want Representation::Tagged() Done. https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/18596005/diff/22001/src/hydrogen-instructions... src/hydrogen-instructions.h:4982: On 2013/07/09 08:34:39, titzer wrote: > Whitespace changes. On purpose, code style. https://codereview.chromium.org/18596005/diff/22001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/18596005/diff/22001/src/hydrogen.h#newcode241 src/hydrogen.h:241: bool Done() { return instr_ == NULL; } On 2013/07/09 08:34:39, titzer wrote: > While you're here, would you mind adding "inline" and "const" keywords? Done.
Message was sent while issue was closed.
Committed patchset #5 manually as r15624 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
