Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10440087:
Compute liveness of instructions to get better and fewer variable names. (Closed)
|
Created:
8 years, 6 months ago by ngeoffray Modified:
8 years, 6 months ago CC:
reviews_dartlang.org, karlklose, ahe Visibility:
Public. |
DescriptionCompute liveness of instructions to get better and fewer variable names.
Committed: https://code.google.com/p/dart/source/detail?r=8145
Patch Set 1 : #
Total comments: 105
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 6 (0 generated)
LGTM. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/codegen.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:171: * Instead we declare them at the end of the function Move this comment too. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:246: SsaLiveIntervalBuilder intervalBuilder = new SsaLiveIntervalBuilder(compiler).visitGraph(graph); https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:458: buffer.add("var "); The 'var' is emitted conditionally. This means that we are not always in STATE_DECLARATION. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:947: emitAssignment(copy, destination); I find it easier to read if the arguments are inversed: emitAssignment(destination, copy); https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:963: if (currentLocation[current] !== null Is this test necessary? Clearly: current == currentLocation[initialValue[current]] means that the assignment has succeeded. If that's not the case then we must be in a cycle, and we shouldn't need to test if a variable was the source of an assignment. If I'm right maybe change the comment to: If the assignment has been done (current == currentLocation[initialValue[current]]) we are done with this variable. Otherwise we have a cycle that we need to break. Note, that only variables that are used as source can be in cycles and we therefore do this (cheaper) check first. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/variable_allocator.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:1: /** copyright. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:30: * Update all ranges that are contained in [start, end[ to have This will probably confuse the doc-generator, and I think US uses a different way of denoting inclusive/exclusive. Personally I'm fine with this notation, though ;) https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:31: * die at [end]. "have die"? either have "end", or just "to die" https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:35: if (range.start >= start && range.end < end) { minor, but personally I prefer start <= range.start && range.end < end. For me it makes it more obvious that range.start..end is enclosed. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:52: for (LiveRange range in ranges) { Haven't yet looked at the use-sites, but this looks like an O(n^2) thingy... https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:66: * The [LiveEnvironment] class contains the live-ins of a basic block. please rename all "live-in"s with "live-interval"s in the comments. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:88: * the map contains the ids where the instructions die. contain https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:90: final Map<HInstruction, int> lives; could we rename this to "lastUsedAt" ? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:119: // If [lastId] is null, then this instruction is bot being used. not https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:134: lives.putIfAbsent(instruction, () => userId); Add comment that reminds that we are visiting the graph in post-order, and that we see a variable first when it dies. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:143: if (existingId == endId) return; Add comment when this can happen. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:285: var source; final HInstruction ? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:361: * collide. Not that it matters, but this should fit on one line. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:440: void freeName(HInstruction instruction) { Add comment: "Frees the [instruction]'s name so it can be used for other instructions". (Just to make sure nobody reads the method as "getNextFreeName") https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:506: * Retrurns whether [instruction] dies at the instruction [at]. Returns
LGTM. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/codegen.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:440: return; Maybe change the structure so that you check for STATE_FIRST_EXPRESSION first and then check for !STATE_FIRST_DECLARATION and do buffer.add there? Add a comment that explains why you don't need an expression separator. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/variable_allocator.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:6: // [end] is not final because it can be udpated due to loops. udpated -> updated https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:12: String toString() { => https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:30: * Update all ranges that are contained in [start, end[ to have to have die? to have died? to die? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:69: /** It would be nice with some better explanations of what these ids are used for. It looks like it's a completely artificial id based on the counter in the interval builder, but it would be nice to know what it's used for here -- and what the significance of the integer value is. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:80: * List of loop markers that will be updated once the loop header is Maybe explain what it marks (what's the int value used for)? Marker is a very generic term. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:90: final Map<HInstruction, int> lives; lives -> liveUntil? How does this relate to the last end position of the ranges in the liveIntervals? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:119: // If [lastId] is null, then this instruction is bot being used. is bot -> is not https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:232: counter--; This is the only place you change counter? Hmm. This looks fishy. If it is intentional to let counter go negative it deserves a comment. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:285: var source; Types on source and destination? I'm pretty sure they are instructions. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:318: String toString() { => https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:329: final Map<HBasicBlock, CopyHandler> copies; This really don't hold copies -- it holds handlers. I would rename it to avoid confusion. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:340: CopyHandler getCopies(HBasicBlock block) { I would probably call this getCopyHandler. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:393: int i = 1; Add comment about not using t0 because it's SWAP_TEMP. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:493: * have no users or that are generated at use site does not need a name. use site does -> use site do. Maybe change method name to needsName to make it fit better with allocateName and freeName? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:497: // TODO(ngeoffray): We need a name for parameters, but we could The TODO comment isn't easy to understand. The "we need a name for parameters" part I get, but is the TODO about allocating such names before getting to this point (and then return false from needsVariable in the future)? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:503: Too many newlines. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:506: * Retrurns whether [instruction] dies at the instruction [at]. Retrurns -> Returns https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:509: LiveInterval atRange = liveIntervals[at]; You call them ranges in a few places, but the type calls the concept intervals. Maybe call them atInterval and instructionInterval? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:518: if (needsVariable(input) && diesAt(input, instruction)) { Add a comment here. If we're the last use of something that has needs a variable, we free it?
Thanks Kasper and Florian! http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... File lib/compiler/implementation/ssa/codegen.dart (right): http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:171: * Instead we declare them at the end of the function On 2012/05/30 18:45:03, floitsch wrote: > Move this comment too. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:246: SsaLiveIntervalBuilder intervalBuilder = On 2012/05/30 18:45:03, floitsch wrote: > new SsaLiveIntervalBuilder(compiler).visitGraph(graph); No, I need the object to take its liveInstructions and liveIntervals fields. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:440: return; On 2012/05/31 05:26:03, kasperl wrote: > Maybe change the structure so that you check for STATE_FIRST_EXPRESSION first > and then check for !STATE_FIRST_DECLARATION and do buffer.add there? Add a > comment that explains why you don't need an expression separator. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:458: buffer.add("var "); On 2012/05/30 18:45:03, floitsch wrote: > The 'var' is emitted conditionally. This means that we are not always in > STATE_DECLARATION. After the first declaration we must go into STATE_DECLARATION, because we cannot introduce a 'var' anymore, and we need a comma to separate the expressions. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:947: emitAssignment(copy, destination); On 2012/05/30 18:45:03, floitsch wrote: > I find it easier to read if the arguments are inversed: > emitAssignment(destination, copy); Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:963: if (currentLocation[current] !== null On 2012/05/30 18:45:03, floitsch wrote: > Is this test necessary? Yes, it is. Because I don't remove a variable that is being 'ready' from the todo list. Consider the example: a = b; c = b; a and c will be ready and in the todo list. Once their assignments have been done, we fetch 'c' from the todo list and the current location of the initial value of 'c' is 'a'. > > Clearly: current == currentLocation[initialValue[current]] means that the > assignment has succeeded. > If that's not the case then we must be in a cycle, and we shouldn't need to test > if a variable was the source of an assignment. > > If I'm right maybe change the comment to: > If the assignment has been done (current == > currentLocation[initialValue[current]]) we are done with this variable. > Otherwise we have a cycle that we need to break. > Note, that only variables that are used as source can be in cycles and we > therefore do this (cheaper) check first. Improved the comment. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... File lib/compiler/implementation/ssa/variable_allocator.dart (right): http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:1: /** On 2012/05/30 18:45:03, floitsch wrote: > copyright. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:6: // [end] is not final because it can be udpated due to loops. On 2012/05/31 05:26:03, kasperl wrote: > udpated -> updated Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:12: String toString() { On 2012/05/31 05:26:03, kasperl wrote: > => Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:30: * Update all ranges that are contained in [start, end[ to have On 2012/05/31 05:26:03, kasperl wrote: > to have die? to have died? to die? to die. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:30: * Update all ranges that are contained in [start, end[ to have On 2012/05/30 18:45:03, floitsch wrote: > This will probably confuse the doc-generator, and I think US uses a different > way of denoting inclusive/exclusive. > Personally I'm fine with this notation, though ;) Let's see how it complains :) http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:31: * die at [end]. On 2012/05/30 18:45:03, floitsch wrote: > "have die"? > either have "end", or just "to die" Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:35: if (range.start >= start && range.end < end) { On 2012/05/30 18:45:03, floitsch wrote: > minor, but personally I prefer start <= range.start && range.end < end. > For me it makes it more obvious that range.start..end is enclosed. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:52: for (LiveRange range in ranges) { On 2012/05/30 18:45:03, floitsch wrote: > Haven't yet looked at the use-sites, but this looks like an O(n^2) thingy... An instruction doesn't have that many live ranges. It is not one live range per block. For example if it survives an if/else, it has one live range. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:66: * The [LiveEnvironment] class contains the live-ins of a basic block. On 2012/05/30 18:45:03, floitsch wrote: > please rename all "live-in"s with "live-interval"s in the comments. It's not containing the live-intervals, it's containing the liveIns set. Changed the comment. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:69: /** On 2012/05/31 05:26:03, kasperl wrote: > It would be nice with some better explanations of what these ids are used for. > It looks like it's a completely artificial id based on the counter in the > interval builder, but it would be nice to know what it's used for here -- and > what the significance of the integer value is. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:80: * List of loop markers that will be updated once the loop header is On 2012/05/31 05:26:03, kasperl wrote: > Maybe explain what it marks (what's the int value used for)? Marker is a very > generic term. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:88: * the map contains the ids where the instructions die. On 2012/05/30 18:45:03, floitsch wrote: > contain Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:90: final Map<HInstruction, int> lives; On 2012/05/30 18:45:03, floitsch wrote: > could we rename this to "lastUsedAt" ? It's really a map of live instructions with the information of where they are last used. I'd prefer using liveInstructions instead. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:90: final Map<HInstruction, int> lives; On 2012/05/31 05:26:03, kasperl wrote: > lives -> liveUntil? How does this relate to the last end position of the ranges > in the liveIntervals? It will be used when adding a range to the live interval of an instruction. Added a comment. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:119: // If [lastId] is null, then this instruction is bot being used. On 2012/05/30 18:45:03, floitsch wrote: > not Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:119: // If [lastId] is null, then this instruction is bot being used. On 2012/05/31 05:26:03, kasperl wrote: > is bot -> is not Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:134: lives.putIfAbsent(instruction, () => userId); On 2012/05/30 18:45:03, floitsch wrote: > Add comment that reminds that we are visiting the graph in post-order, and that > we see a variable first when it dies. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:143: if (existingId == endId) return; On 2012/05/30 18:45:03, floitsch wrote: > Add comment when this can happen. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:232: counter--; On 2012/05/31 05:26:03, kasperl wrote: > This is the only place you change counter? Hmm. This looks fishy. If it is > intentional to let counter go negative it deserves a comment. Changed the name to instructionId, and added a comment at the declaration of the field. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:285: var source; On 2012/05/31 05:26:03, kasperl wrote: > Types on source and destination? I'm pretty sure they are instructions. They are here, but will be String in codegen :) This class is really a std::pair equivalent. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:285: var source; On 2012/05/30 18:45:03, floitsch wrote: > final HInstruction ? Made it final, but not typed, as they can also be String (see sequentializeParallelCopies in codegen). http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:318: String toString() { On 2012/05/31 05:26:03, kasperl wrote: > => Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:329: final Map<HBasicBlock, CopyHandler> copies; On 2012/05/31 05:26:03, kasperl wrote: > This really don't hold copies -- it holds handlers. I would rename it to avoid > confusion. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:340: CopyHandler getCopies(HBasicBlock block) { On 2012/05/31 05:26:03, kasperl wrote: > I would probably call this getCopyHandler. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:361: * collide. On 2012/05/30 18:45:03, floitsch wrote: > Not that it matters, but this should fit on one line. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:393: int i = 1; On 2012/05/31 05:26:03, kasperl wrote: > Add comment about not using t0 because it's SWAP_TEMP. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:440: void freeName(HInstruction instruction) { On 2012/05/30 18:45:03, floitsch wrote: > Add comment: "Frees the [instruction]'s name so it can be used for other > instructions". > > (Just to make sure nobody reads the method as "getNextFreeName") Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:493: * have no users or that are generated at use site does not need a name. On 2012/05/31 05:26:03, kasperl wrote: > use site does -> use site do. Maybe change method name to needsName to make it > fit better with allocateName and freeName? Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:497: // TODO(ngeoffray): We need a name for parameters, but we could On 2012/05/31 05:26:03, kasperl wrote: > The TODO comment isn't easy to understand. The "we need a name for parameters" > part I get, but is the TODO about allocating such names before getting to this > point (and then return false from needsVariable in the future)? The problem is that we're making parameters generate at use site, and I would like to avoid that to not special case parameters. I improved the TODO comment. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:503: On 2012/05/31 05:26:03, kasperl wrote: > Too many newlines. Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:506: * Retrurns whether [instruction] dies at the instruction [at]. On 2012/05/30 18:45:03, floitsch wrote: > Returns Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:506: * Retrurns whether [instruction] dies at the instruction [at]. On 2012/05/31 05:26:03, kasperl wrote: > Retrurns -> Returns Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:509: LiveInterval atRange = liveIntervals[at]; On 2012/05/31 05:26:03, kasperl wrote: > You call them ranges in a few places, but the type calls the concept intervals. > Maybe call them atInterval and instructionInterval? Done. http://codereview.chromium.org/10440087/diff/2001/lib/compiler/implementation... lib/compiler/implementation/ssa/variable_allocator.dart:518: if (needsVariable(input) && diesAt(input, instruction)) { On 2012/05/31 05:26:03, kasperl wrote: > Add a comment here. If we're the last use of something that has needs a > variable, we free it? Done.
Haven't understood it all yet, so these are mainly style comments. https://chromiumcodereview.appspot.com/10440087/diff/2001/frog/tests/leg/type... File frog/tests/leg/type_inference_test.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/frog/tests/leg/type... frog/tests/leg/type_inference_test.dart:71: new RegExp("sum = sum \\+ i"); This is completely static text, so a RegExp is overdoing it. Does String have a .contains you could use, or if not, could you use .indexof(...) >= 0? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/codegen.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:149: final Set<String> declaredVariables; Can you add a doc-comment describing what this means (and ditto for delayedVariableDeclarations, so the distinction is clear) https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:156: VariableNames variableNames; And this one too, to explain the difference from the other ones. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:246: SsaLiveIntervalBuilder intervalBuilder = It's also used below. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:440: return; Why this change? Where do you change the state to STATE_DECLARATION? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:895: * Sequentialize a list of parallel copies. Describe which criteria are being used to do the ordering/sequentializing. "list of parallel copies" => "list of conceptually parallel copyings" (a list is already sequential). Change "copy" to "copying" (and pluralize accordingly). A "copy" is the result of the act of "copying". It's the acts that you sequentialize. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:899: Map<String, String> currentLocation = new Map<String, String>(); What is a "location" here? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:905: List<String> todo = <String>[]; "todo" -> "worklist". https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:907: // List of variables that we can assign a value to (ie are not Describe which criteria are being used to do the ordering/sequentializing. "list of parallel copies" => "list of conceptually parallel copyings" (a list is already sequential). Change "copy" to "copying" (and pluralize accordingly). A "copy" is the result of the act of "copying". It's the acts that you sequentialize. Or are they "assignments"? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/variable_allocator.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:284: class Copy { Rename to "Copying" (or "Assignment" if that isn't taken). This represents an act of copying, not the resulting copy. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:302: * Trivial assignments from a constant to the phi of a successor. How is this different from a copying? I.e., what makes them trivial? https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:330: static final String SWAP_TEMP = 't0'; How do we know this doesn't collide with anything? (If we do, write it here). https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:535: names.addAssignment(predecessor, input, phi); If it doesn't need a variable, what are we assigning to?
Thanks Lasse for the comments. https://chromiumcodereview.appspot.com/10440087/diff/2001/frog/tests/leg/type... File frog/tests/leg/type_inference_test.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/frog/tests/leg/type... frog/tests/leg/type_inference_test.dart:71: new RegExp("sum = sum \\+ i"); On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > This is completely static text, so a RegExp is overdoing it. Does String have a > .contains you could use, or if not, could you use .indexof(...) >= 0? Done. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/codegen.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:149: final Set<String> declaredVariables; On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > Can you add a doc-comment describing what this means (and ditto for > delayedVariableDeclarations, so the distinction is clear) Done. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:156: VariableNames variableNames; On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > And this one too, to explain the difference from the other ones. Done. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:440: return; On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > Why this change? > Where do you change the state to STATE_DECLARATION? In declareVariable line 454. I need this change because the 'var' is not needed when the variable following has already been declared. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:895: * Sequentialize a list of parallel copies. On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > Describe which criteria are being used to do the ordering/sequentializing. > "list of parallel copies" => "list of conceptually parallel copyings" (a list is > already sequential). > Change "copy" to "copying" (and pluralize accordingly). A "copy" is the result > of the act of "copying". It's the acts that you sequentialize. Added the 'conceptually'. As discussed, kept the 'copy/copies' terminology due to (mis-)uses in the literature. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:899: Map<String, String> currentLocation = new Map<String, String>(); On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > What is a "location" here? It is a variable. It really is "the variable that holds the value of that variable". https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:905: List<String> todo = <String>[]; On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > "todo" -> "worklist". Done. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/codegen.dart:907: // List of variables that we can assign a value to (ie are not On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > Describe which criteria are being used to do the ordering/sequentializing. > "list of parallel copies" => "list of conceptually parallel copyings" (a list is > already sequential). > Change "copy" to "copying" (and pluralize accordingly). A "copy" is the result > of the act of "copying". It's the acts that you sequentialize. Or are they > "assignments"? Done. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... File lib/compiler/implementation/ssa/variable_allocator.dart (right): https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:284: class Copy { On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > Rename to "Copying" (or "Assignment" if that isn't taken). > This represents an act of copying, not the resulting copy. As discussed, kept the name :) https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:302: * Trivial assignments from a constant to the phi of a successor. On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > How is this different from a copying? I.e., what makes them trivial? Because the source does not need a name. Added a comment. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:330: static final String SWAP_TEMP = 't0'; On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > How do we know this doesn't collide with anything? > (If we do, write it here). Done. And added a TODO to deal with parameters. https://chromiumcodereview.appspot.com/10440087/diff/2001/lib/compiler/implem... lib/compiler/implementation/ssa/variable_allocator.dart:535: names.addAssignment(predecessor, input, phi); On 2012/05/31 08:24:56, Lasse Reichstein Nielsen wrote: > If it doesn't need a variable, what are we assigning to? The instruction (eg a constant, or a generate at use site instruction). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
