> 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. > > Steve Reinhardt wrote: > 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?
Oh, but you're using the trie to manage matches on different page sizes, right? Forgot about that. Never mind. - 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
