Hi Sophiane,
Thanks for the contribution. It looks like some of the patch doesn't apply cleanly in reviewboard. Did you use the hg postreview extension? It may also help to use the "-o" option on the extension.
Cheers!

I don't like the variable names, I think it's confusing especially in the Python part which is the user part. "lookup_latency"  does not clearly refer to the tag lookup action , and "ram_latency" is also not very clear. Maybe something like "tag_latency" and "line_latency" could be better ? I think the two parts of a cache are well identified in this example.

src/mem/cache/tags/base.cc (Diff revision 7)

more than 80 chars in the line

Show all issues

src/mem/cache/tags/base_set_assoc.hh (Diff revision 7)

Bad white spaces around here.

Show all issues

src/mem/cache/tags/fa_lru.cc (Diff revision 7)

Indentation is is off.

Show all issues

Thanks for contributing this. Could you please change the commit message such that the summary (first line) is no more than 65 characters and the body no more than 75 characters. The commit summary should start with the mem keyword.

src/mem/cache/tags/base_set_assoc.hh (Diff revision 8)

Would it make sense to move most of this code in the constructor? The flag sequentialAccess and the condition lookupLatency >= dataLantency shouldn't change during the simulation.

Show all issues

1. Sophiane SENNI June 28, 2016, 2:19 a.m. (June 28, 2016, 2:19 a.m.)

   I think this code should be left here. Because the total access latency now depends on both tag_latency and data_latency, the value of the 'lat' parameter specifying the access latency is not correct when the accessBlock() function is called. As a result, the 'lat' value should be updated inside the function depending on if it is a sequential or parallel access.

2. Sophiane SENNI June 30, 2016, 3:30 a.m. (June 30, 2016, 3:30 a.m.)

   Regarding your suggestion, I am agree with you that it would be better to move the code in the constructor since as you mentioned the flag sequentialAccess and the access latency value should not change during the simulation. I will see how I can modify the patch accordingly.

3. Nikos Nikoleris June 30, 2016, 5:25 a.m. (June 30, 2016, 5:25 a.m.)

   Thanks for doing this! I think you could create a new variable in the base class (BaseTags) and use that as the latency on every access. In any case, in the code the latency is not dependent on the replacement policy. Mind though that if the access is a miss the latency should always be tagLatency, even when we've enabled the parallel access. We could also move the sequentialAccess variable to BaseCache although I am not sure how applicable a parallel lookup to the tag and the data array is for a fully associative cache.

4. Nikos Nikoleris July 17, 2016, 12:42 p.m. (July 17, 2016, 12:42 p.m.)

   Sophiane, how are things progressing? It would be really great to get done with this and commit it.

5. Sophiane SENNI July 18, 2016, 1:10 a.m. (July 18, 2016, 1:10 a.m.)

   Hi Nikos,
   I was and I am still quite busy with some other work I have to finish. But I will try to publish the new version of the patch as soon as possible. Sorry for the delay.

6. Sophiane SENNI July 20, 2016, 3:45 a.m. (July 20, 2016, 3:45 a.m.)

   Nikos,
   For the new patch, I use the "accessLatency" variable, as the latency on every access, in the base class (BaseTags). I tried to initialize this variable in the constructor according to the access mode as follows:
   BaseTags::BaseTags(const Params *p)
   
       : ClockedObject(p), blkSize(p->block_size), size(p->size),
   
         lookupLatency(p->tag_latency), dataLatency(p->data_latency),
   
         accessLatency(p->sequential_access ? 
   
         // If sequential access, sum tag lookup and data access latencies
   
                       (p->tag_latency + p->data_latency) :
   
         // If parallel access, take the max latency
   
         // between tag lookup and data access               
   
                       (p->tag_latency >= p->data_latency ? 
   
                         p->tag_latency : p->data_latency )),
   
         cache(nullptr), warmupBound(0),
   
         warmedUp(false), numBlocks(0)
   
   {
   
   }
   However, when checking by simulation, the value of "sequential_access" parameter is always taken as "False". Even if this paramater is set to "True" in configs/common/Caches.py
   Do you have a solution to correctly initialize the "accessLatency" variable in the initialization list ?
   Thanks.

7. Nikos Nikoleris July 20, 2016, 5:54 a.m. (July 20, 2016, 5:54 a.m.)

   Hi Sohpianne, Can you post the patch so that I can have a look? I guess you run into an ordering problem with how we initialize the objects. BTW instead of the a >= b ? a  : b you can always use the std::max(a, b)

src/mem/cache/tags/fa_lru.cc (Diff revision 8)

Same here

Show all issues

1. Sophiane SENNI June 28, 2016, 2:19 a.m. (June 28, 2016, 2:19 a.m.)

   Same here

Just some some minor style issues.

src/mem/cache/tags/base_set_assoc.hh (Diff revision 8)

spurious new line

Show all issues

src/mem/cache/tags/base_set_assoc.hh (Diff revision 8)

Spurious new line

Show all issues

src/mem/cache/tags/base_set_assoc.hh (Diff revision 8)

Please wrap the comment such it does not violate the 79 char per line limit

Show all issues

src/mem/cache/tags/fa_lru.cc (Diff revision 8)

Please wrap the comment such it does not violate the 79 char per line limit

Show all issues

src/mem/cache/tags/fa_lru.cc (Diff revision 8)

spurious new line

Show all issues

src/mem/cache/tags/fa_lru.cc (Diff revision 8)

spurious new line

Show all issues

src/mem/cache/tags/fa_lru.cc (Diff revision 8)

Please wrap the comment such it does not violate the 79 char per line limit

Show all issues

Sorry, that was probably unclear from my comment but the commit message body should be wrapped such that no line is more than 75 characters rather than the whole body being limited to 76 char.

Thanks for all the effort you've put into this. Please make this last set of changes and it should be good to go.

src/mem/cache/tags/Tags.py (Diff revision 11)

This stale argument causes the problems you're facing please remove it, I don't think it is needed

Show all issues

1. Sophiane SENNI July 25, 2016, 6:20 a.m. (July 25, 2016, 6:20 a.m.)

   You are right. This change resolved the problem. Thanks Nikos.

src/mem/cache/tags/base.cc (Diff revision 11)

Please remove the comment here. If you would like move the comment in the header to explain what the access latency is.

Show all issues

src/mem/cache/tags/base.cc (Diff revision 11)

no need for parenthesis here

Show all issues

src/mem/cache/tags/base.cc (Diff revision 11)

you don't need the parenthesis here over

Show all issues

Ship It!

Ship It!

src/mem/cache/tags/Tags.py (Diff revision 12)

Why would the tags care about the data latency?

Show all issues

1. Sophiane SENNI July 28, 2016, 2:55 a.m. (July 28, 2016, 2:55 a.m.)

   This is required to initialize the "accessLatency" variable in tags/base.cc
   
   If I am right, the "accessLatency" variable declared in tags/base.hh is used to consider the cache latency on every access (i.e. considering tags and data accesses). Previously, "accessLatency" was always assigned to "hit_latency". With this patch, "accessLatency" value is initialized according to the cache access mode (parallel or sequential).

src/mem/cache/tags/base.hh (Diff revision 12)

Seems odd that the tags need to track this. Is this still the best division? Perhaps explain why it's here.

Show all issues

1. Sophiane SENNI July 28, 2016, 2:55 a.m. (July 28, 2016, 2:55 a.m.)

   This can be removed. We actually do not need it since data latency is already included in accessLatency.

src/mem/cache/tags/base_set_assoc.hh (Diff revision 12)

Could you add a comment here?
It seems to me this code is not right, as it checks if the data is technically written now, but we only need the data at time T.
Should we not rather add the dataLatency to the blk->whenReady and then do the plus or max opteration?

Show all issues

1. Sophiane SENNI July 28, 2016, 2:55 a.m. (July 28, 2016, 2:55 a.m.)

   I actually don't really understand what this code represents, which was already present before applying the patch. Because it seems to show a case where the cache latency is greater than accessLatency, when the lat variable is updated as follows:
   
   lat = cache->ticksToCycles(blk->whenReady - curTick())
   
   Can this situation actually occur ?

2. Andreas Hansson Aug. 25, 2016, 2:58 a.m. (Aug. 25, 2016, 2:58 a.m.)

   blk->whenReady represents the fact that the block is technically not available yet. Due to how we do timing modelling we annotate the block when it arrives, but have to remember when it is actually availalbe. Thus, anything we do here should add on top of the blk->whenReady. Same for fa_lru

3. Sophiane SENNI Aug. 26, 2016, 3:17 a.m. (Aug. 26, 2016, 3:17 a.m.)

   Ok. So if I understood, we actually need to apply the accessLatency on top of the blk->whenReady. Hence, the good code would be as follows:
   if (blk->whenReady > curTick()
   
                   && cache->ticksToCycles(blk->whenReady - curTick())
   
                   > accessLatency) {
   
                   lat = cache->ticksToCycles(blk->whenReady - curTick()) + accessLatency;
   
               }
   Does this change make more sense ?

4. Andreas Hansson Oct. 24, 2016, 1:41 p.m. (Oct. 24, 2016, 1:41 p.m.)

   Yes. Also, could you add a comment to explain what is happening here.

src/mem/cache/tags/fa_lru.cc (Diff revision 12)

here we don't care about blk->whenReady?

Show all issues

1. Sophiane SENNI July 28, 2016, 2:55 a.m. (July 28, 2016, 2:55 a.m.)

   I we care about blk->whenReady in base_set_assoc.hh, I assume we have also to care about it here.

2. Andreas Hansson Oct. 24, 2016, 1:41 p.m. (Oct. 24, 2016, 1:41 p.m.)

   Agreed.

Some minor concerns that hopefully make sense.

Hi,

Someone can commit this patch ? I don't have right access on the repository, either Sophiane.
Thank you.

src/mem/cache/tags/base_set_assoc.hh (Diff revision 15)

If you don't mind changing it, could we keep the operator on the first line?
The same goes for the condition in the if statement.
I am not sure if this is actually captured in the style, but it is definitley convention.
The same goes for the copy in FA-LRU
Thanks!

Show all issues

thanks for getting this in shape