Hi Nilay, I apologize for the delay, but I was mostly travelling / in meetings last week and I didn't have a chance to review your patches and emails until this morning.
Overall, your patches are definitely solid steps in the right direction and your profiling data sounds very promising. If you get the chance, please send it to me. I would be interested to know what are the top performance bottlenecks after your change. Before you spend time converting the other protocols, I do want to discuss the three points you brought up last week (see below). I have a bunch of free time over the next three days (Mon. - Wed.) and I do think a telephone conversation is best to discuss these details. Let me know what times work for you. Brad 1. Currently the implicit TBE and Cache Entry pointers are set to NULL in the calls to doTransition() function. To set these, we would need to make calls to a function that returns the pointer if the address is in the cache, NULL otherwise. I think we should retain the getEntry functions in the .sm files for in case of L1 cache both instruction and the data cache needs to be checked. This is something that I probably would prefer keeping out of SLICC. In fact, we should add getEntry functions for TBEs where ever required. These getEntry would now return a pointer instead of a reference. We would need to add support for return_by_pointer to SLICC. Also, since these functions would be used inside the Wakeup function, we would need to assume a common name for them across all protocols, just like getState() function. [BB] I would be very interested why you believe we should keep the getEntry functions out of SLICC. In my mind, this is one of the few functions that is very consistent across protocols. As I mentioned before, I really want to keep any notion of pointers out of the .sm files and avoid the changes you are proposing to getCacheEntry. We should probably discuss this in detail over-the-phone. 2. I still think we would need to change the changePermission function in the CacheMemory class. Presently it calls findTagInSet() twice. Instead, we would pass on the CacheEntry whose permissions need to be changed. This would save one call. We should also put the variable m_locked in the AbstractCacheEntry (may be make it part of the permission variable) to avoid the second call. [BB] I like moving the locked field to AbstractCacheEntry and removing the separate m_locked data structure. However, just a minor point, but we should avoid duplicating code in CacheMemory to support this change. Other than that, this looks good to me. 3. In the getState() and setState() functions, we need to specify that the function assumes that implicit TBE and CacheEntry pointers have been passed as arguments. How should we do this? I think we would need to push them in to the symbol table before they can be used in side the function. [BB] I'm a little confused by your current patch. It appears that you are proposing having two pairs of getState and setState functions. I would really like to avoid that and just have one pair of getState and setState functions. Also when I say implicitly pass the TBE and CacheEntry pointers, I mean that for the actions (similar to address). However, I think it is fine to explicitly pass these parameters into getState and setState (also similar to Address and State). _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
