> On April 14, 2016, 3:32 p.m., Joel Hestness wrote: > > I'm really not a fan of the way we're trying to do this. To summarize, this > > change distributes address mapping parameters through all of the Ruby > > controllers, just so that they can call a global function that takes those > > as parameters and returns the mapping. > > > > This leads to a few problems. First, we'll have to change all of the > > protocol files, and get the correct parameters to the correct controllers > > in config files. This will be painful for users with protocols outside of > > mainline, because they need to reason about how to calculate correct > > parameters. Second, it's a confusing way to distribute mapping parameters; > > they are only used in calls to mapAddressToRange, which is a global > > function. The fact that mapAddressToRange is global indicates that there a > > number of ways we could organize this, some of which would likely avoid > > spreading parameters throughout protocol files. Finally, this is a very > > fragile way to handle component mapping, so it won't be extensible. What do > > I do if I notice that I need more complicated parameterization of the > > directory mapping function? It looks like I'd have to extend the function > > signature for mapAddressToRange to take more parameters, and then > > distribute more parameters through the relevent protocol files. This sounds > > very disruptive. > > > > I think we should be aiming to implement this by moving mapAddressToRange > > into the RubySystem. Then, we can just give each RubySystem instance the > > number of controllers that it needs to map, and it can configure all of the > > appropriate mapping functions and parameters. The existing mapping > > functions used in protocol files can indirectly call the RubySystem's > > mapping functions as appropriate, so we shouldn't need to change protocol > > files (or if we do, it would be a 1-time change). This also encapsulates > > the mapping functionality in a single location, so modifying mapping > > functions and parameters should be far more extensible. > > Brandon Potter wrote: > I think that it might be reasonable to move the component mapping > functionality into RubySystem because the controllers already have a handle > for RubySystem. However, I have a couple concerns: > > _"What do I do if I notice that I need more complicated parameterization > of the directory mapping function? It looks like I'd have to extend the > function signature for mapAddressToRange to take more parameters, and then > distribute more parameters through the relevent protocol files. This sounds > very disruptive."_ > > Can you provide an example of what you're suggesting for the directory > mapping? I'm not saying that it's impossible/unreasonable, but I am not aware > of what you'd want to use as the other mapping methods. Currently, we should > be able to support the address interleaving method using mapAddressToRange > (which looks like a banked cache/memory) and a direct method where a private > cache sends to a downstream private cache. We could extend it further if > needed so that we have a private cache send to a second private cache that is > banked. I believe that would match the functionality that the classic cache > model provides. (Andreas, feel free to correct me if I'm mistaken; I'm not > very familiar with classic.) > > _"Then, we can just give each RubySystem instance the number of > controllers that it needs to map, and it can configure all of the appropriate > mapping functions and parameters."_ > > By default, RubySystem does not track the controllers; the controllers > were previously tracked in a global. With my first attempt at this problem, I > moved that global into RubySystem and the controllers were added into it as > they were initialized. The RubySystem was aware of the controllers and could > handle queries to with the Machine* functions because they were being routed > through the RubySystem into it's controller vector. The controller vector was > a 2-D array which consisted of a vector of the MachineTypes each of which > held a vector of controllers for that given MachineType. So, it's possible to > do as you're suggesting, although it's not as simple as you're making it out > to be. > > _END CONCERNS_ > > If and only if we find that the mapAddressToRange is not flexible enough, > I'm willing to try an alternative that you suggest. First, please look at > http://reviews.gem5.org/r/3113/ again. Before I do any more coding on this, I > want a clear-cut path that everyone agrees on for a component mapping. I am > not going to develop another implementation unless there's total agreement on > how the solution looks. Andreas, a more flexible implementation probably > won't be using AddrRange. If you really believe that Ruby should use it, you > might speak up here. Also, you might have an opinion on why it's sufficient > for classic (if I understand everything correctly).
>> "What do I do if I notice that I need more complicated parameterization of >> the directory mapping function? It looks like I'd have to extend the >> function signature for mapAddressToRange to take more parameters, and then >> distribute more parameters through the relevent protocol files. This sounds >> very disruptive." > Can you provide an example of what you're suggesting for the directory > mapping? I'm not saying that it's impossible/unreasonable, but I am not aware > of what you'd want to use as the other mapping methods. Currently, we should > be able to support the address interleaving method using mapAddressToRange > (which looks like a banked cache/memory) and a direct method where a private > cache sends to a downstream private cache. We could extend it further if > needed so that we have a private cache send to a second private cache that is > banked. I believe that would match the functionality that the classic cache > model provides. (Andreas, feel free to correct me if I'm mistaken; I'm not > very familiar with classic.) As a practical example, in gem5-gpu, we have a really hacky solution for complicated bit transformations to avoid pathological memory access patterns. A common issue is when GPU threads process rows of a matrix: Suppose a row is 4096 bytes (12 address bits between row starts), and the directory address mapping high bit is the 11th bit (0-indexed). Then, all GPU memory accesses in a small time window can end up going to the same directory and cause huge contention. We deal with this in gem5-gpu by swizzling bits above and below the mapping "high bit", but parameterizing and modifying the current directory mapping scheme was *exceptionally* error prone, and I don't want to ever modify the mapping again. More abstractly, we should be aiming to augment Ruby so that we can define and swap out nearly arbitrary mapping functions. If we home the address mapping functions in a single place, like the RubySystem, then we can easily parameterize the mapping function selection (e.g. similar to passing a cache replacement policy to a cache). As Andreas suggests, I too believe the AddrRange functionality can be eventually used in the mapping functionality (note: AddrRanges have 4 parameters that define the mapping, indicating it is desirable to have more flexibility than Ruby mappings). To me, this is very desirable functionality. >> "Then, we can just give each RubySystem instance the number of controllers >> that it needs to map, and it can configure all of the appropriate mapping >> functions and parameters." > By default, RubySystem does not track the controllers; the controllers were > previously tracked in a global. With my first attempt at this problem, I > moved that global into RubySystem and the controllers were added into it as > they were initialized. The RubySystem was aware of the controllers and could > handle queries to with the Machine* functions because they were being routed > through the RubySystem into it's controller vector. The controller vector was > a 2-D array which consisted of a vector of the MachineTypes each of which > held a vector of controllers for that given MachineType. So, it's possible to > do as you're suggesting, although it's not as simple as you're making it out > to be. I don't think any of my proposed solutions will require the RubySystem to have pointers to the separate controllers. To handle address mapping, it will just need the count of each controller type for address striping across them. The address mapping functions can still pass MachineIDs or NetDests back to the calling controllers. > If and only if we find that the mapAddressToRange is not flexible enough, I'm > willing to try an alternative that you suggest. First, please look at > http://reviews.gem5.org/r/3113/ again. Before I do any more coding on this, I > want a clear-cut path that everyone agrees on for a component mapping. I am > not going to develop another implementation unless there's total agreement on > how the solution looks. Andreas, a more flexible implementation probably > won't be using AddrRange. If you really believe that Ruby should use it, you > might speak up here. Also, you might have an opinion on why it's sufficient > for classic (if I understand everything correctly). I believe everything I'm describing in this current set of reviews is consistent with my recommendations in 3113. We can aim for a solution that uses AddrRanges in later iterations - we need more flexible address mapping structure first. > On April 14, 2016, 3:32 p.m., Joel Hestness wrote: > > configs/ruby/GPU_RfO.py, line 475 > > <http://reviews.gem5.org/r/3433/diff/1/?file=54622#file54622line475> > > > > Please maintain spacing around '='. It's really hard to read something > > like tcc_low_bit=block_size_bits, and it's inconsistent with spacing > > through the rest of the file (e.g. Cluster constructors) > > Brandon Potter wrote: > Maybe I am interpreting it wrong but doesn't this style guide rule apply > here? > http://www.m5sim.org/Coding_Style - "no space around '=' when used in > parameter/argument lists, either to bind default parameter values (in Python > or C++) or to bind keyword arguments (in Python)" Ah, apologies. I wasn't aware we had adopted the Python/Google style for this. I dropped this issue. Eventually, we'll need to make '=' spacing consistent within this file, but note that our use of '=' spacing in named parameters lists is quite inconsistent throughout gem5. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3433/#review8222 ----------------------------------------------------------- On April 4, 2016, 11:43 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3433/ > ----------------------------------------------------------- > > (Updated April 4, 2016, 11:43 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11435:9b3faddc9c0b > --------------------------- > ruby: improved component mapping > > This patch aims are improving how addresses are mapped to different entities > in the memory system. The main issue was having a static member function in > the DirectoryMemory class and several different globally visible functions > almost doing the same thing. The functions map_Address_to_DirectoryNode, > map_Address_to_Directory, and broadcast() are being dropped. There is no > replacement provided for broadcast(), but one should be able to use the one > already available provided in the NetDest class. The functions > map_Address_to_DirectoryNode and map_Address_to_Directory should be replaced > with mapAddressToRange(). > > > Diffs > ----- > > src/mem/protocol/RubySlicc_Util.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/DirectoryMemory.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/DirectoryMemory.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/DirectoryMemory.py > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/Prefetcher.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-probeFilter.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_directory-L2cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_directory-dma.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_token-L1cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_token-L2cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_token-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_token-dma.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_hammer-cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_hammer-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_hammer-dma.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/Network_test-cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/RubySlicc_ComponentMapping.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-Region-CorePair.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-Region-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-RegionBuffer.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-RegionDir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Two_Level-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Two_Level-dma.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MI_example-cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MI_example-dma.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-CorePair.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-L3cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Two_Level-L1cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Two_Level-L2cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_VIPER_Region-TCC.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Three_Level-L1cache.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_VIPER-TCP.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_VIPER-SQC.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_VIPER-TCC.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_RfO-TCCdir.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_RfO-TCP.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_RfO-SQC.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/GPU_RfO-TCC.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MI_example.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MOESI_AMD_Base.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MOESI_CMP_directory.py > cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MOESI_CMP_token.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MOESI_hammer.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/Network_test.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/GPU_VIPER_Baseline.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/GPU_VIPER_Region.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MESI_Three_Level.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/MESI_Two_Level.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/GPU_RfO.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > configs/ruby/GPU_VIPER.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > > Diff: http://reviews.gem5.org/r/3433/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
