Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10382234:
Introduce locations based code generation. (Closed)
|
Created:
8 years, 7 months ago by Vyacheslav Egorov (Google) Modified:
8 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionIntroduce locations based code generation.
Instructions and computations switched to the new code generation scheme should implement two virtual functions:
- locs() should return pointer to a LocationSummary object, that specifies location constraints for the instruction/computation.
- EmitNativeCode() should emit instructions/computations native code using locations provided through LocationSummary object attached to the instruction.
For converted functions instruction pattern should be removed from the FlowGraphCompiler visitor. It will not be used as long as instruction/computation returns non-NULL from locs().
Committed: https://code.google.com/p/dart/source/detail?r=7818
Patch Set 1 #
Total comments: 39
Patch Set 2 : addressed Srdjan comments #
Total comments: 22
Messages
Total messages: 8 (0 generated)
Srdjan & Florian, I have been experimenting with various ways we can use to gradually introduce new location based code generation scheme. This attempt seems the most appealing to me. I converted one instruction StrictEquals. With automatic register allocation code looks like [notice %rcx instead of %rdx] 0x007fdf171000d0 00000090 59 pop %rcx 0x007fdf171000d1 00000091 58 pop %rax 0x007fdf171000d2 00000092 48 3b c1 cmp %rcx,%rax 0x007fdf171000d5 00000095 75 0c jne 0xa3 0x007fdf171000d7 00000097 48 b8 21 b1 2e 1c df mov $0x7fdf1c2eb121,%rax 0x007fdf171000de 0000009e 7f 00 00 0x007fdf171000e1 000000a1 eb 0a jmp 0xad 0x007fdf171000e3 000000a3 48 b8 01 b1 2e 1c df mov $0x7fdf1c2eb101,%rax 0x007fdf171000ea 000000aa 7f 00 00 0x007fdf171000ed 000000ad 50 push %rax This CL is a request for comments. There are still some things I could not figure out: for example where should we put platform specific locations specification. Currently I just placed a method on instruction itself and implemented it in the flow_graph_compiler_x64.h but this looks very bad. (disregard the fact that location information looks very verbose at the moment, once we start converting all instructions we can reduce verbosity with helper methods). One of the goal I had before myself is to allow incremental conversion of the codegenerator to a new location based generation hence LocationSummaries to allow unconverted instructions remain the same (just return NULL from GetLocationSummary() method).
I like the overall approach. The LocationSummary is fine as a temporary vehicle. Maybe add a intermediate_language_x64.cc to put the register constraints for each platform are defined by implementing the MakeLocationSummary-functions. On 2012/05/18 03:34:27, Vyacheslav Egorov (Google) wrote: > Srdjan & Florian, > > I have been experimenting with various ways we can use to gradually introduce > new location based code generation scheme. This attempt seems the most appealing > to me. > > I converted one instruction StrictEquals. With automatic register allocation > code looks like [notice %rcx instead of %rdx] > > 0x007fdf171000d0 00000090 59 pop %rcx > 0x007fdf171000d1 00000091 58 pop %rax > 0x007fdf171000d2 00000092 48 3b c1 cmp %rcx,%rax > 0x007fdf171000d5 00000095 75 0c jne 0xa3 > 0x007fdf171000d7 00000097 48 b8 21 b1 2e 1c df mov $0x7fdf1c2eb121,%rax > 0x007fdf171000de 0000009e 7f 00 00 > 0x007fdf171000e1 000000a1 eb 0a jmp 0xad > 0x007fdf171000e3 000000a3 48 b8 01 b1 2e 1c df mov $0x7fdf1c2eb101,%rax > 0x007fdf171000ea 000000aa 7f 00 00 > 0x007fdf171000ed 000000ad 50 push %rax > > This CL is a request for comments. There are still some things I could not > figure out: for example where should we put platform specific locations > specification. Currently I just placed a method on instruction itself and > implemented it in the flow_graph_compiler_x64.h but this looks very bad. > > (disregard the fact that location information looks very verbose at the moment, > once we start converting all instructions we can reduce verbosity with helper > methods). > > One of the goal I had before myself is to allow incremental conversion of the > codegenerator to a new location based generation hence LocationSummaries to > allow unconverted instructions remain the same (just return NULL from > GetLocationSummary() method).
I think this is the right direction. Here are a bunch of comments and ideas, feel free to discuss in person. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... File runtime/vm/flow_graph_compiler_x64.cc (right): http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:554: LocationSummary* StrictCompareComp::MakeLocationSummary() { In unoptimizing compiler this is the same for all StrictCompareComps. Instead of allocating every time, maybe preallocate and reuse the LocationSummary. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:559: 1, UnallocatedLocation(UnallocatedLocation::REGISTER)); Too verbose. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:562: return summary; As you said, this does not belong here. Maybe in an architecture dependent location or intermediate_language_xxx.cc file. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:572: Register result = ToRegister(comp->GetLocationSummary()->result_location()); Too verbose, maybe: comp->locs()->in(0)->AsRegister(); http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:1145: static Register AllocateFreeRegister(bool* blocked_registers) { I would rather use BitVector or GrowableArray since they have range checks. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:1149: return static_cast<Register>(regno); The register allocation needs also to move out of code generator. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... File runtime/vm/intermediate_language.h (right): http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1392: return NULL; Will we need this? I would guess that we are only interested in the output(s) of LocationSummary. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1840: All these classes need a small amount of comment, describing their intent. Without them I am not certain what they are supposed to do. Also, they seem complex enough to be located in their own file. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1842: class Location : public ValueObject { Add comment about formatting. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1847: REGISTER enums are typically named kInvalid, kUnallocated etc. Should it also contain kStack or kAddress? http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1854: uword value() const { return value_; } This belongs in protected? http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1857: explicit Location(uword value) : value_(value) { } Do you need this constructor? http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1860: : value_(KindField::encode(kind) | PayloadField::encode(payload)) { } BitField::encode needs range checks. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1868: uword value_; const http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1885: static UnallocatedLocation Cast(Location loc) { Maybe AsUnaloocatedLocation instead of Cast? And doesn't it belong into Location? http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1899: explicit RegisterLocation(Register reg) This cannot handle XMMRegisters, maybe add the register kind? http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1916: class LocationSummary : public ZoneAllocated { I am not too happy with the name LocationSummary as it sounds like the result of an aggregation. Maybe ComputationLocations? http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1920: Zone* zone = Isolate::Current()->current_zone(); Use ZoneGrowableArray fro now, and we can replace it with newly to be added ZoneArray which does all the range checks and correct allocation.
Hi Srdjan, I tried to address your comments. Please take another look [I also left some things unaddressed please find my comments inline] http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... File runtime/vm/flow_graph_compiler_x64.cc (right): http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:554: LocationSummary* StrictCompareComp::MakeLocationSummary() { On 2012/05/18 18:00:14, srdjan wrote: > In unoptimizing compiler this is the same for all StrictCompareComps. Instead of > allocating every time, maybe preallocate and reuse the LocationSummary. Preallocating and reusing proves to be hard because it's a zone object so I can't just save it into some static field inside the instruction class. The only way to share it apparently is to introduce opcodes and store precomputes location summaries in a table (opcode -> locs) inside the compiler. Seems a bit complicated but I could not come up with anything better. What do you think? http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:559: 1, UnallocatedLocation(UnallocatedLocation::REGISTER)); On 2012/05/18 18:00:14, srdjan wrote: > Too verbose. introduced a helper function and shortened method names http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:562: return summary; On 2012/05/18 18:00:14, srdjan wrote: > As you said, this does not belong here. Maybe in an architecture dependent > location or intermediate_language_xxx.cc file. moved to intermediate_language_x64.cc http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:572: Register result = ToRegister(comp->GetLocationSummary()->result_location()); On 2012/05/18 18:00:14, srdjan wrote: > Too verbose, maybe: > comp->locs()->in(0)->AsRegister(); Done. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:1145: static Register AllocateFreeRegister(bool* blocked_registers) { On 2012/05/18 18:00:14, srdjan wrote: > I would rather use BitVector or GrowableArray since they have range checks. both are zone allocated which seems unnecessary expensive to me (will require allocation per instruction) decided to use EmbeddedVector instead. http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:1149: return static_cast<Register>(regno); On 2012/05/18 18:00:14, srdjan wrote: > The register allocation needs also to move out of code generator. Moved simple register allocator into LocationSummary itself (and LocationSummary was moved into locations.{h,cc} http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... File runtime/vm/intermediate_language.h (right): http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1392: return NULL; On 2012/05/18 18:00:14, srdjan wrote: > Will we need this? I would guess that we are only interested in the output(s) of > LocationSummary. Some instructions also pop and push things (see for example ReThrow). Thus is seems that they need a full location information not just output. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1840: On 2012/05/18 18:00:14, srdjan wrote: > All these classes need a small amount of comment, describing their intent. > Without them I am not certain what they are supposed to do. > Also, they seem complex enough to be located in their own file. Commented, moved to location.{cc,h} http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1842: class Location : public ValueObject { On 2012/05/18 18:00:14, srdjan wrote: > Add comment about formatting. Done. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1847: REGISTER On 2012/05/18 18:00:14, srdjan wrote: > enums are typically named kInvalid, kUnallocated etc. Should it also contain > kStack or kAddress? Renamed to match style. Did not add kStack/kAddress because I think that I should not add anything that would not be used (and thus tested) in this CL. We should try to grow Location functionality organically (it definitely will become more rich as we add more sofisticated instructions/register allocation) http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1854: uword value() const { return value_; } On 2012/05/18 18:00:14, srdjan wrote: > This belongs in protected? Can't be in the protected because it is used from static methods in derived classes which does not work. [this is a very strange part of C++: derived classes have access to protected members if and only if access is performed through an object of derived type. if method on derived class tries to call a protected method on an object of base class then compiler will complain, more information is in standard itself: see section 11.5] http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1857: explicit Location(uword value) : value_(value) { } On 2012/05/18 18:00:14, srdjan wrote: > Do you need this constructor? It is called by similar constructors of children which are used in Cast methods. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1860: : value_(KindField::encode(kind) | PayloadField::encode(payload)) { } On 2012/05/18 18:00:14, srdjan wrote: > BitField::encode needs range checks. It does use is_valid(), I think that should be enough. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1868: uword value_; On 2012/05/18 18:00:14, srdjan wrote: > const If I put const here I would need to implement my own assignment operator which writes into a const field. I don't think it's worth it. There is no public setter should nobody would modify it. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1885: static UnallocatedLocation Cast(Location loc) { On 2012/05/18 18:00:14, srdjan wrote: > Maybe AsUnaloocatedLocation instead of Cast? And doesn't it belong into > Location? I can put helper methods into Location but then I will still need Cast methods to implement casting in a sane way. I don't want to implement it as: *reinterpret_cast<UnallocatedLocation*>(this) Alternative approach would be to use fake this pointers and pass Location* everywhere instead of Location (in a similar fashion to V8's tagged pointers) but this is very ugly. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1899: explicit RegisterLocation(Register reg) On 2012/05/18 18:00:14, srdjan wrote: > This cannot handle XMMRegisters, maybe add the register kind? see above about organically growing Location as we start using more complicated functionality. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1916: class LocationSummary : public ZoneAllocated { On 2012/05/18 18:00:14, srdjan wrote: > I am not too happy with the name LocationSummary as it sounds like the result of > an aggregation. Maybe ComputationLocations? ComputationLocations does not work because instructions can also have one. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1920: Zone* zone = Isolate::Current()->current_zone(); On 2012/05/18 18:00:14, srdjan wrote: > Use ZoneGrowableArray fro now, and we can replace it with newly to be added > ZoneArray which does all the range checks and correct allocation. Using ZoneGrowableArray. Added TODO to use ZoneArray.
LGTM with comments http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... File runtime/vm/flow_graph_compiler_x64.cc (right): http://codereview.chromium.org/10382234/diff/1/runtime/vm/flow_graph_compiler... runtime/vm/flow_graph_compiler_x64.cc:554: LocationSummary* StrictCompareComp::MakeLocationSummary() { On 2012/05/20 12:07:46, Vyacheslav Egorov (Google) wrote: > On 2012/05/18 18:00:14, srdjan wrote: > > In unoptimizing compiler this is the same for all StrictCompareComps. Instead > of > > allocating every time, maybe preallocate and reuse the LocationSummary. > > Preallocating and reusing proves to be hard because it's a zone object so I > can't just save it into some static field inside the instruction class. > > The only way to share it apparently is to introduce opcodes and store > precomputes location summaries in a table (opcode -> locs) inside the compiler. > Seems a bit complicated but I could not come up with anything better. What do > you think? Valid points, good suggestion. Let's not do anything now. http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... File runtime/vm/intermediate_language.h (right): http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1392: return NULL; On 2012/05/20 12:07:46, Vyacheslav Egorov (Google) wrote: > On 2012/05/18 18:00:14, srdjan wrote: > > Will we need this? I would guess that we are only interested in the output(s) > of > > LocationSummary. > > Some instructions also pop and push things (see for example ReThrow). Thus is > seems that they need a full location information not just output. > Please add comment why this is needed (essentially what you replied) http://codereview.chromium.org/10382234/diff/1/runtime/vm/intermediate_langua... runtime/vm/intermediate_language.h:1854: uword value() const { return value_; } On 2012/05/20 12:07:46, Vyacheslav Egorov (Google) wrote: > On 2012/05/18 18:00:14, srdjan wrote: > > This belongs in protected? > > Can't be in the protected because it is used from static methods in derived > classes which does not work. > > [this is a very strange part of C++: derived classes have access to protected > members if and only if access is performed through an object of derived type. if > method on derived class tries to call a protected method on an object of base > class then compiler will complain, more information is in standard itself: see > section 11.5] Ha! http://codereview.chromium.org/10382234/diff/2002/runtime/vm/flow_graph_compi... File runtime/vm/flow_graph_compiler_x64.cc (right): http://codereview.chromium.org/10382234/diff/2002/runtime/vm/flow_graph_compi... runtime/vm/flow_graph_compiler_x64.cc:551: UNREACHABLE(); Why is that (add comment)? Wait, this is part of the new code emission structure? http://codereview.chromium.org/10382234/diff/2002/runtime/vm/flow_graph_compi... runtime/vm/flow_graph_compiler_x64.cc:1123: } else? UNIMPLEMENTED? check location is stack OK? http://codereview.chromium.org/10382234/diff/2002/runtime/vm/intermediate_lan... File runtime/vm/intermediate_language.h (right): http://codereview.chromium.org/10382234/diff/2002/runtime/vm/intermediate_lan... runtime/vm/intermediate_language.h:178: } Maybe we should get rid of operators (some other CL). http://codereview.chromium.org/10382234/diff/2002/runtime/vm/locations.cc File runtime/vm/locations.cc (right): http://codereview.chromium.org/10382234/diff/2002/runtime/vm/locations.cc#new... runtime/vm/locations.cc:14: for (int regno = 0; regno < kNumberOfCpuRegisters; regno++) { intptr_t http://codereview.chromium.org/10382234/diff/2002/runtime/vm/locations.h File runtime/vm/locations.h (right): http://codereview.chromium.org/10382234/diff/2002/runtime/vm/locations.h#newc... runtime/vm/locations.h:23: // register number). What is register number? CPU register-number, for now, I guess. http://codereview.chromium.org/10382234/diff/2002/runtime/vm/locations.h#newc... runtime/vm/locations.h:70: Policy policy() { return PolicyField::decode(payload()); } const? http://codereview.chromium.org/10382234/diff/2002/runtime/vm/locations.h#newc... runtime/vm/locations.h:150: ZoneGrowableArray<Location> input_locations_; This is a value object -> GrowableArray instead of ZoneGrowableArray.
LGTM. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... File runtime/vm/locations.cc (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.cc:15: if (!blocked_registers->At(regno)) { (*blocked_registers)[regno] should also work here. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.cc:16: blocked_registers->SetAt(regno, true); (*blocked_registers)[regno] = true; should also work here. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/locations.h File runtime/vm/locations.h (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:52: uword value_; I think Location should be the same size on all platforms. Maybe use uint64_t instead?
https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... File runtime/vm/locations.cc (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.cc:15: if (!blocked_registers->At(regno)) { On 2012/05/21 18:33:38, Florian Schneider wrote: > (*blocked_registers)[regno] should also work here. I like the idea of getting rid of const overloaded operators and use At and SetAt, but keeping an open mind :-). Maybe we should agree on using one, and ditch the other? https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/locations.h File runtime/vm/locations.h (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:52: uword value_; On 2012/05/21 18:33:38, Florian Schneider wrote: > I think Location should be the same size on all platforms. Maybe use uint64_t > instead? What is the advantage of having them the same size on all platforms? The advantage of using uword is that it makes it much more efficient to pass around on 32-bit architectures.
Resolved comments. Landed. Now we can proceed with instruction convertion. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/flow_gra... File runtime/vm/flow_graph_compiler_x64.cc (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/flow_gra... runtime/vm/flow_graph_compiler_x64.cc:551: UNREACHABLE(); On 2012/05/21 16:05:22, srdjan wrote: > Why is that (add comment)? Wait, this is part of the new code emission > structure? Yes, native template were moved. I added a comment. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/flow_gra... runtime/vm/flow_graph_compiler_x64.cc:1123: } On 2012/05/21 16:05:22, srdjan wrote: > else? UNIMPLEMENTED? check location is stack OK? Added assert. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... File runtime/vm/locations.cc (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.cc:14: for (int regno = 0; regno < kNumberOfCpuRegisters; regno++) { On 2012/05/21 16:05:22, srdjan wrote: > intptr_t Done. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.cc:15: if (!blocked_registers->At(regno)) { On 2012/05/21 18:33:38, Florian Schneider wrote: > (*blocked_registers)[regno] should also work here. I'll go with a method, it looks nicer. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.cc:16: blocked_registers->SetAt(regno, true); On 2012/05/21 18:33:38, Florian Schneider wrote: > (*blocked_registers)[regno] = true; > > should also work here. I'll go with a method, it looks nicer. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/locations.h File runtime/vm/locations.h (right): https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:23: // register number). On 2012/05/21 16:05:22, srdjan wrote: > What is register number? CPU register-number, for now, I guess. changed to register code and added reference to Register enum https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:52: uword value_; On 2012/05/21 18:33:38, Florian Schneider wrote: > I think Location should be the same size on all platforms. Maybe use uint64_t > instead? Added a TODO, we will select size once we know the encoding (and how big bitfields are better be). https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:52: uword value_; On 2012/05/21 19:49:43, srdjan wrote: > On 2012/05/21 18:33:38, Florian Schneider wrote: > > I think Location should be the same size on all platforms. Maybe use uint64_t > > instead? > > What is the advantage of having them the same size on all platforms? > The advantage of using uword is that it makes it much more efficient to pass > around on 32-bit architectures. Fixed size is less error prone (it is easier to reason individual size of bit-fields that constitute uword). For now I left it uword and added TODO-comment that we can resolve later. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:70: Policy policy() { return PolicyField::decode(payload()); } On 2012/05/21 16:05:22, srdjan wrote: > const? Done. https://chromiumcodereview.appspot.com/10382234/diff/2002/runtime/vm/location... runtime/vm/locations.h:150: ZoneGrowableArray<Location> input_locations_; On 2012/05/21 16:05:22, srdjan wrote: > This is a value object -> GrowableArray instead of ZoneGrowableArray. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
