----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/916/#review1718 -----------------------------------------------------------
Overall this looks great. Thanks to Andreas and all the other ARM folks that have worked on this. There are definitely some tricky issues coming up with the larger port reworking, but this set of patches doesn't touch on any of them, so it's a good place to start! I have three main areas of concern: 1. Procedural issues. I already pointed out the couple of files that should really be treated as renames and not deletes and additions. Also I'm not sure about merging these into a single changeset for commit. Splitting it up definitely made it easier to review (thanks!) but led to some anomalies that I've pointed out in the detailed reviews, including some places where the final code is suboptimal. I can also see where doing the file renames I mentioned and also making each incremental patch compilable would be in conflict, which also argues for a single changeset. However it would be a big one, and you'd lose the "understandability" of the incremental changes. Have you guys thought about this? Any other opinions? 2. SE/FS convergence. I generally hesitate to push people to incorporate orthogonal code cleanup into unrelated changes, but this seems like a really great opportunity to push toward unifying the two TranslatingPort classes. At the very least I think it would be good to rename both SETranslatingPort and FSTranslatingPort to just TranslatingPort, with the one you get depending on your compilation mode. Then in other parts of the code you wouldn't really have a distinction... for example, see my comment on that snippet in remote_gdb.cc. Also renaming the SE-mode getMemProxy() to getVirtProxy() to be parallel with FS mode would help. Since you're renaming things anyway it seems like we should do it right. Of course this needs to be coordinated with what Gabe is doing; how are you dealing with the VirtualPort/TranslatingPort issue, Gabe? 3. Giving access to the System port. If you just have more objects that need a System pointer so they can find the System proxy port, then we should do that using existing techniques in Python. If you really need to expose the configuration hierarchy in C++ (which is probably a good idea, though I don't think it's appropriate for the current purpose), then we should do that in a cleaner fashion than what the current code does. Let me know if you have any questions... Steve - Steve On 2011-11-28 10:16:33, Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/916/ > ----------------------------------------------------------- > > (Updated 2011-11-28 10:16:33) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > MEM: Add the port proxies to the source tree > > Port proxies are used to replace non-structural ports, and thus enable > all ports in the system to correspond to a structural entity. > > The following replacements are going to follow in the next patches: > FunctionalPort > PortProxy > TranslatingPort > SETranslatingProxy > VirtualPort > FSTranslatingProxy > > This patch does not instantiate any of the aforementioned ports, it > merely adds the source files. > > > Diffs > ----- > > src/mem/SConscript e70d031cb5f9 > src/mem/fs_translating_proxy.hh PRE-CREATION > src/mem/fs_translating_proxy.cc PRE-CREATION > src/mem/port_proxy.hh PRE-CREATION > src/mem/port_proxy.cc PRE-CREATION > src/mem/se_translating_proxy.hh PRE-CREATION > src/mem/se_translating_proxy.cc PRE-CREATION > > Diff: http://reviews.m5sim.org/r/916/diff > > > Testing > ------- > > util/regress passing all ignoring failing eio and t1000 > > > Thanks, > > Andreas > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
