> On April 8, 2012, 5:33 p.m., Steve Reinhardt wrote: > > src/sim/addrtrie.hh, line 38 > > <http://reviews.gem5.org/r/1141/diff/1/?file=25679#file25679line38> > > > > What about templating on the key type too (Addr)? Then it would be a > > totally generic trie and should definitely go in base and not sim. > > Gabe Black wrote: > It would be slightly more generic, but really you'd just be able to > change the size of the integer key. It wouldn't work with, for instance, a > string key, or if you wanted the most specific match like for IP routing, > etc. I'll do it anyway since it's not a totally unreasonable thing to do, and > someone may make it more genuinely generic in the future.
Just because something's a template parameter doesn't mean you can bang in any old type... even the ability to use different width integers seems reasonable (if say there's some reason to want a 32-bit and a 64-bit version in the same code). There should probably be a comment at the top mentioning that Key should be an integer type. > On April 8, 2012, 5:33 p.m., Steve Reinhardt wrote: > > src/sim/addrtrie.hh, line 144 > > <http://reviews.gem5.org/r/1141/diff/1/?file=25679#file25679line144> > > > > Not sure if the official style guide says anything, but my gut (and I > > think emacs) would line up the two goesAfter() calls, not offset them by > > one space. > > Gabe Black wrote: > This is because it's on the next line, and I think my editor > automatically indented it two stops/eight spaces. That happens to almost but > not quite line up with the line above. I don't care greatly which way it's > done, so I'll line it up. Thanks. > On April 8, 2012, 5:33 p.m., Steve Reinhardt wrote: > > src/sim/addrtrie.hh, line 207 > > <http://reviews.gem5.org/r/1141/diff/1/?file=25679#file25679line207> > > > > I don't see what you gain by separating lookup() and lookupNode(), > > since the latter is only called from the former, and the separation > > requires an extra check of node for being non-null. > > Gabe Black wrote: > If you don't record the Node pointer when you insert something, you need > a way to get at it later so you can delete that mapping. Otherwise there's no > way to retrieve the Node, just the value it contains. I'm pretty sure the > compiler will inline one into the other anyway and be able to get rid of the > NULL check. That's sort of what I figured was going on initially, except that remove() never calls lookupNode(), so I thought it was pointless... but now I see from the X86 patch that lookupNode() is part of the external interface. That seems a bit undesirable; I would think that ideally the Node class would not be visible outside the trie. Can we define an interface that makes that true? I think if you rewrite TLB::demapPage() as TlbEntry *entry = trie.lookup(va); if (entry) { trie.remove(entry->trieNode); entry->trieNode = NULL; freeList.push_back(entry); } then there would be no need for lookupNode() in the external interface (or as a separate function at all), and no point in the X86 code where you dereference a Node*. Then the Node class itself could be private, and Node* could be an opaque handle type. That said, it makes me wonder how much faster the trie is than a hash_map... have you looked at that? - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1141/#review2510 ----------------------------------------------------------- On April 10, 2012, 11:04 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1141/ > ----------------------------------------------------------- > > (Updated April 10, 2012, 11:04 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 8943:524c4b58c710 > --------------------------- > sim: A trie data structure specifically to speed up paging lookups. > > This change adds a trie data structure which stores an arbitrary pointer type > based on an address and a number of relevant bits. Then lookups can be done > against the trie where the tree is traversed and the first legitimate match > found is returned. > > > Diffs > ----- > > src/base/trie.hh PRE-CREATION > src/unittest/SConscript 5534a564f6a0 > src/unittest/trietest.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/1141/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
