Make CacheTagArray great again (#221, #225) #257

denislos · 2017-12-03T12:41:17Z

I have created BPUCache.h and LRUTagCache.h to implement BPU cache. I have also created some unit tests for it. Actually, there are still tests (read_no_touch test, write test, etc) to be created because I have added virtually only those ones which are tailored to the LRUTagCache.

pavelkryukov · 2017-12-03T14:33:51Z

Why did you remove CacheTagArray?? We need that class for dozens of caches we will implement later.

denislos · 2017-12-03T16:05:15Z

I made a mistake when I renamed it. However, I think it's the way CacheTagArray should look like when hash tables are used. I dropped "bits and bytes" things because I thought it would be better to work with entries in the cache. I haven't changed the logic.

So this is how BPUCache ( my CacheTagArray) works. In my opinion, It's also the way original CacheTagArray works.
Each set is an array of ways. We count sets from 0 to number_of_sets - 1 and we also count ways from 0 to number_of_ways - 1. When a new element is about to be added, its set is evaluated in dependence on the address we pass as an argument to write method, but its way is the way we decide to use in accordance with the LRU algorithm which is implemented independently for each set.

pavelkryukov · 2017-12-03T17:24:04Z

simulator/bpu/bpu.h

@@ -162,7 +146,7 @@ class BPFactory {
             std::exit( EXIT_FAILURE);
        }

-        return map.at( name)->create( size_in_entries, ways, branch_ip_size_in_bits);
+        return map.at( name)->create( size_in_entries, ways);


How should we configure # of bits for IP now?

pavelkryukov · 2017-12-03T17:33:45Z

I dropped "bits and bytes" things

What do you mean by "bits and bytes"?

I thought

Please, contact me first before making "revolutionary" changes. For the most of cases the things are made here due to a kind of "strategic" plans. I have some experience in writing simulators and I know already that something would be wrong ideas. Cache-which-come-together-with-data is one of them.

it would be better to work with entries in the cache

For many purposes we do not need to model entries in the cache, we may have only tags (for example, data cache — the data is actually taken

Each set is an array of ways. We count sets from 0 to number_of_sets - 1 and we also count ways from 0 to number_of_ways - 1. When a new element is about to be added, its set is evaluated in dependence on the address we pass as an argument to write method, but its way is the way we decide to use in accordance with the LRU algorithm which is implemented independently for each set.

Right, that is how cache works.

pavelkryukov · 2017-12-03T17:50:50Z

simulator/infra/cache/t/miss_rate_sim.cpp

-    /* Same as previous for full-associative cache. */
-    for ( auto cache_size : cache_sizes)
-    {
-        CacheTagArray cta( 1024 * cache_size, 1024 * cache_size / 4);


That was deleted with nothing in return.

I have said in the pull request description that there are some tests to be created so miss_rate_sim converted to GTest is one of them. I have planned to use it as the main and the only test for key methods like write and read_no_touch. However, I thought it would be enough to have bpu tests and LRUTagCache tests in the initial version.

denislos · 2017-12-03T19:25:29Z

By the "bits and bytes" things I mean arguments like cache_size_in_bits and addr_size_in_bits which are not needed if the size in entries is used. I have removed branch_ip_size_in_bits for the same reason. What do you mean by cache-which-comes-together-with-data?
Sorry for not asking you first, I thought it would much better if we had more abstract CacheTagArray class, however, I still don't get why the other approach is better in our situation because we do not use explicitly any of them now (e.g. branch_ip_size_in_bits) ( we use default parameters). If we are not ok with default cell structure of CacheTagArray in the future and do not benefit from its simplicity, we still will be able to convert every structure to it by ourselves. Sorry for this.

pavelkryukov · 2017-12-03T20:10:08Z

simulator/bpu/bpu.h

@@ -43,42 +43,33 @@ template<typename T>
 class BP final: public BaseBP
 {
    std::vector<std::vector<T>> data;
-    CacheTagArray tags;
+    CacheTagArray cache;


Then it should be 'tags'

pavelkryukov · 2017-12-03T20:10:44Z

simulator/bpu/bpu.h

-        { }
+    BP( uint32 size_in_entries, uint32 ways)
+        : data( ways, std::vector<T>( size_in_entries / ways))
+        , cache( size_in_entries, ways)


Please restore 'branch_ip_size_in_bits'

pavelkryukov · 2017-12-03T20:11:12Z

simulator/bpu/bpu.h

-              // we're reusing existing CacheTagArray functionality,
-              // but here we don't split memory in blocks, storing
-              // IP's only, so hardcoding here the granularity of 4 bytes:
-              4,


Where did that comment go?

pavelkryukov · 2017-12-03T20:12:10Z

simulator/bpu/bpu.h

-              // we're reusing existing CacheTagArray functionality,
-              // but here we don't split memory in blocks, storing
-              // IP's only, so hardcoding here the granularity of 4 bytes:
-              4,


What happened to "4"? That line size is used for BPU, but not for data caches.

pavelkryukov · 2017-12-03T20:13:09Z

simulator/infra/cache/LRUTagCache.h

+        std::unordered_map<Key, typename std::list<Key>::const_iterator> lru_hash{};
+
+        size_type number_of_elements = 0u;
+        size_type CAPACITY;


Is there any use case to set "size_type" to anything other than "size_t"?

It looks better for me not to convert size_t to uint32 and introduce size_type.

I think uint32 should be used, as we will never have a cache with # of sets or ways exceeding the 32 bit.

pavelkryukov · 2017-12-03T20:32:06Z

simulator/infra/cache/cache_tag_array.cpp


-std::pair<bool, uint32> CacheTagArray::read( Addr addr)
+std::pair<Addr, uint32> CacheTagArray::read( Addr addr)


What does Addr mean here? There are only two results possible: cache hit (true) and cache miss (false).

Sorry, It's definitely a mistake

pavelkryukov · 2017-12-03T20:39:57Z

By the "bits and bytes" things I mean arguments like cache_size_in_bits and addr_size_in_bits which are not needed if the size in entries is used.

They are still needed as real HW does aliasing to reduce HW cost. For example, BP may use only 16 LSB of IP. Sometimes it makes a collision if there are two instructions with same 16 LSB, but it happens rarely while much HW is reduced.

pavelkryukov · 2017-12-03T20:44:34Z

we do not use explicitly any of them now

It is not the reason to remove anything. There are a lot of stubs which may be unused at the moment, but they are required on the next iteration of development. If I have to remove something, I’ll remove it by myself or create a new task for student. If you think that something should be removed, please consult me/code owner first, as there might be a reason to keep it.

pavelkryukov · 2017-12-03T20:46:45Z

it would much better if we had more abstract CacheTagArray

We had it as much abstract as possible, excepting the LRU policy. By adding entries to the CacheTagArray, it becomes a Cache class which has less flexibility (you have to handle the data)

pavelkryukov · 2017-12-03T20:49:20Z

What do you mean by cache-which-comes-together-with-data?

To keep entries (BP entries, for instance) and cache tags in the same class. As I explained, for many cases we can have cache tags only.

pavelkryukov · 2017-12-04T09:16:57Z

simulator/infra/cache/cache_tag_array.h

+                       uint32 size_of_line = 4,
+                       uint32 addr_size_in_bits = 32)
+            : Log( false)
+            , number_of_sets( check_arguments( size_in_bytes, 


Could you please explain the reason why you removed CacheTagArrayCheck class?

Why do we need the separate class just to check arguments?

Why not? C++ guarantees that ctor of CacheTagArrayCheck class is executed before anything else, so if arguments are ill-formed, we won't go inside the function and get any undefined behavior. Additionally, if an alternative implementation of CacheTagArray existed (some unusual cache), programmer would inherit from CacheTagArrayCheck and explicitly get all the checks. Instead, your function "check_arguments" may be easily dropped by accident, plus it has very unclear return value (I would expect void or Boolean true if everything correct at least).

pavelkryukov · 2017-12-04T09:17:22Z

simulator/infra/cache/cache_tag_array.h

        std::pair<bool, uint32> read_no_touch( Addr addr) const;
-        /* create new entry in cache */


Deleting valid comments is a sabotage.

pavelkryukov · 2017-12-04T09:19:41Z

simulator/infra/cache/cache_tag_array.h

 #include <infra/types.h>
 #include <infra/log.h>

-/* Replacement algorithm modules (LRU). */
-class LRUInfo


LRU was intentionally put to the separate class as there are a lot of alternative policies (optimal replacement, pseudo-LRU, MRU, weighted-LRU etc.). So, please put the separate class back, later we will introduce the polymorphic replacement policy selection.

pavelkryukov · 2017-12-04T09:19:59Z

simulator/infra/cache/cache_tag_array.h

@@ -1,82 +1,58 @@
 /**
- * cache_tag_array.h
- * Header for the cache tag array model.


Deleting valid comments is a sabotage.

pavelkryukov · 2017-12-04T09:22:29Z

simulator/infra/cache/cache_tag_array.cpp

-    return addr / line_size;
-}
+    cache[ num_set].update( num_tag);
+    const auto&[ is_hit, value] = cache[ num_set].find( num_tag);


Find is counter-intuition. Usually when we write data to the cache, we should find only a LRU way.

pavelkryukov · 2017-12-04T16:44:01Z

simulator/infra/cache/t/unit_test.cpp

+    std::pair<bool, uint32> result;
+
+    // try to find an element in the empty cache
+    GTEST_ASSERT_NO_DEATH( result = cache.find( 10););


There is no point to have "GTEST_ASSERT_NO_DEATH". If something failed inside, it would crash the test anyway.

We won't be able to tell what exactly went wrong.

In both cases you cannot tell what went wrong without a debugger and/or code review. Wrapping everything into these brackets gives extra ~5% of information, but readability of the code is around zero.

pavelkryukov

Let's do integration step by step. Could you please resubmit review only with the test?

denislos · 2017-12-04T17:45:24Z

Sorry, I don't get it. What do you mean by resubmitting review only with the test? You want me to commit only with the unit tests, right?

pavelkryukov · 2017-12-04T17:47:27Z

Right.

pavelkryukov · 2017-12-05T14:00:22Z

simulator/infra/cache/cache_tag_array.cpp

-        lru.touch( set_num, way_num); // update LRU info
-    }
+    if ( result.first)
+        lru_module.update( num_set, num_tag); // update LRU if it's a hit


# of way should be used here, not a tag, right?

No, we need a tag here.

No, we need not. LRU class should know nothing about tags. It has only two basic operations:

Put some way to the head of the list (== touch)

Take a way from the tail ( == update == allocate)

Then there should be a kind of reflection of WAYS to TAGS( e,g, another map which is not good), because we need somehow to know the least recently used tag to erase it from the hash table.

Well, I will use another map.

Then there should be a kind of reflection of WAYS to TAGS( e,g, another map which is not good), because we need somehow to know the least recently used tag to erase it from the hash table.

Yes, it should be used for CacheTagArray. It is a common operation for cache ("check what data is in way 1 of set 4").

Well, I will use another map.

Note that for PseudoLRU there is no map inside LRU class, it uses a different mechanism. We might use FIFO replacement as well.

I have used a map inside CacheTagArray.

pavelkryukov · 2017-12-05T14:02:02Z

simulator/infra/cache/cache_tag_array.h

        uint32 write( Addr addr);

-        uint32 set( Addr addr) const;
-        Addr tag( Addr addr) const;
+        uint32 set( Addr addr) const { return (addr / line_size) & (number_of_sets - 1); }


Recently I've added aliasing to these functions, please return it back.

pavelkryukov · 2017-12-05T14:03:02Z

simulator/infra/cache/cache_tag_array.h

+            : lru_info( number_of_sets, LRUCacheInfo<Addr>( number_of_ways))
+        { }
+
+        std::pair<Addr, uint32> update( uint32 num_set, Addr addr) 


There should be two methods:

Touch (when an existing element is accessed)

Update (when an new element is added)

pavelkryukov · 2017-12-05T16:25:48Z

simulator/infra/cache/cache_tag_array.cpp

 std::pair<bool, uint32> CacheTagArray::read( Addr addr)
 {
-    const auto lookup_result = read_no_touch( addr);
-    const auto&[ is_hit, way_num] = lookup_result;
+    auto result = read_no_touch( addr);


Could you please use C++17 syntax?

const auto&[ is_hit, way_num] = lookup_result;

pavelkryukov · 2017-12-05T16:28:25Z

simulator/infra/cache/cache_tag_array.cpp

-    for ( auto it = list.begin(); it != list.end(); ++it)
+    std::size_t return_value = number_of_elements;
+
+    if ( number_of_elements == CAPACITY)


I think you may initialize the hash table in ctor and remove number_of_elements

pavelkryukov · 2017-12-05T16:31:01Z

simulator/infra/cache/cache_tag_array.h

+        const Addr   addr_mask;
+
+        // maps to convert num_ways to tags
+        std::vector<std::unordered_map<uint32, Addr>> ways_to_tags;


It can be a std::vector<std::vector<Addr>>, right?

pavelkryukov · 2017-12-05T18:14:47Z

simulator/infra/cache/cache_tag_array.h

+                                  size_of_line,
+                                  addr_size_in_bits)
+            , number_of_sets( size_in_bytes / ( ways * size_of_line))
+            , number_of_ways( ways)


Why were these fields moved from CacheTagArrayCheck?

Why do we need them there? I mean, It's strange to have them in the "Check" class.

Denis, that strange kind of discussion is repeating over and over. You are the person who did the changes, so I should be the guy who expects the answers which explain your motivation.

We need it here because we may have it here. "CacheTagArrayCheck" has the highest level of abstraction, so if a class was inherited from it we would not think about correctness of size variables anymore. We may add more methods to the base class which operate with sizes only and then use them for some constant expressions or run-time checks etc. I agree that "Check" may be not the best name, but "CacheTagArraySizeBaseWithBoundaryChecks" has some drawbacks either.

There are a lot of things in C++ programs which may look strange, but they can be even the only one possible solution originated from days and months of the code evolution. If they don't contain a bug, it is better not to touch them.

pavelkryukov · 2017-12-05T18:16:42Z

simulator/infra/cache/cache_tag_array.cpp

-    if ( line_size == 0)
-        serr << "ERROR: Wrong arguments! Line size should be greater than zero"
+
+    if ( size_of_line == 0)


"line size" is a common idiom for caches, please move it back.

pavelkryukov · 2017-12-05T20:40:38Z

simulator/infra/cache/cache_tag_array.h

+        const Addr   addr_mask;
+
+        // to convert num_ways to tags
+        std::vector<std::vector<std::pair<bool, Addr>>> ways_to_tags;


std::pair is not the best idea.

It is usually unclear what "first" and "second" mean

One day we will need to add one more field to the table. With pair/tuple we shall re-write everything!!

denislos added 3 commits December 3, 2017 15:17

Add bpucache

c1b22ca

Add BPUCache and LRUTagCache

46024a2

Introduce BPUCache to BPU

a6dc528

denislos closed this Dec 3, 2017

Fix problem with clang and msvc

870caf5

denislos reopened this Dec 3, 2017

denislos closed this Dec 3, 2017

denislos reopened this Dec 3, 2017

pavelkryukov suggested changes Dec 3, 2017

View reviewed changes

denislos changed the title ~~Make BPUCache great again (#221, #225, #77)~~ Make CacheTagArray great again (#221, #225, #77) Dec 3, 2017

denislos added 2 commits December 3, 2017 22:30

Undo changes in infrastucture and return miss_rate_sim

090abeb

Merge branch 'master' of https://github.com/MIPT-ILab/mipt-mips into lru

e3b5fdd

pavelkryukov suggested changes Dec 3, 2017

View reviewed changes

denislos added 4 commits December 4, 2017 01:27

Restore dropped arguments and make improvements

0027638

Merge branch 'master' of https://github.com/MIPT-ILab/mipt-mips into lru

69c1739

Fix problem with msvc

9648bca

Fix comment

ae4b649

pavelkryukov suggested changes Dec 4, 2017

View reviewed changes

pavelkryukov reviewed Dec 4, 2017

View reviewed changes

pavelkryukov closed this Dec 4, 2017

denislos added 3 commits December 5, 2017 00:17

Merge branch 'master' into lru

62c02ef

Move LRU to LRUModule class

573786b

Remove LRUTagCache.h

af40453

denislos reopened this Dec 5, 2017

denislos changed the title ~~Make CacheTagArray great again (#221, #225, #77)~~ Make CacheTagArray great again (#221, #225) Dec 5, 2017

pavelkryukov reviewed Dec 5, 2017

View reviewed changes

pavelkryukov suggested changes Dec 5, 2017

View reviewed changes

denislos added 2 commits December 5, 2017 17:54

Make improvements

05ac3a9

Avoid using tags in the LRU module and use an extra map in CacheTagArray

8e0fe99

pavelkryukov reviewed Dec 5, 2017

View reviewed changes

Use C++17 syntax

f906c73

pavelkryukov suggested changes Dec 5, 2017

View reviewed changes

pavelkryukov reviewed Dec 5, 2017

View reviewed changes

denislos added 3 commits December 5, 2017 21:59

Remove number_of_elements and make improvements

47a18ad

Remove number_of_elements

128ec28

Restore CacheTagArrayCheck

bc52f5a

pavelkryukov suggested changes Dec 5, 2017

View reviewed changes

denislos and others added 2 commits December 6, 2017 09:51

Use struct instead of std::pair

95fd3f5

Merge branch 'master' into lru

c686d33

pavelkryukov approved these changes Dec 6, 2017

View reviewed changes

pavelkryukov merged commit 97760b9 into MIPT-ILab:master Dec 6, 2017

denislos deleted the lru branch February 3, 2018 17:42


		std::pair<bool, uint32> CacheTagArray::read( Addr addr)
		std::pair<Addr, uint32> CacheTagArray::read( Addr addr)

		std::pair<bool, uint32> read_no_touch( Addr addr) const;
		/* create new entry in cache */

Make CacheTagArray great again (#221, #225) #257

Make CacheTagArray great again (#221, #225) #257

Conversation

denislos commented Dec 3, 2017

pavelkryukov commented Dec 3, 2017

denislos commented Dec 3, 2017

Choose a reason for hiding this comment

pavelkryukov commented Dec 3, 2017

Choose a reason for hiding this comment

denislos Dec 3, 2017 • edited

Choose a reason for hiding this comment

denislos commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkryukov commented Dec 3, 2017

pavelkryukov commented Dec 3, 2017

pavelkryukov commented Dec 3, 2017

pavelkryukov commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkryukov left a comment

Choose a reason for hiding this comment

denislos commented Dec 4, 2017

pavelkryukov commented Dec 4, 2017

pavelkryukov Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkryukov Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkryukov Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkryukov Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denislos Dec 3, 2017 •

edited

pavelkryukov Dec 5, 2017 •

edited

pavelkryukov Dec 5, 2017 •

edited

pavelkryukov Dec 5, 2017 •

edited

pavelkryukov Dec 5, 2017 •

edited