Igor (#82)

* Igor

* Some refactoring

* Fix tlb addr zeroing + clang-tidy fixes

* Update format-check.yml

* Fix sign

* Add static cast

* Fix cast

* Enable spdlog + add auto

* Fix spdlog

* Fix 8q

---------

Co-authored-by: andrey <derzhavin.andrej@yandex.ru>

.github/workflows/format-check.yml

@@ -8,7 +8,7 @@ on:
 
 jobs:
   formatting-check:
-    name: Formatting Check of ${{ matrix.path }} directory
+    name: Formatting Check of ${{ github.workspace }}/${{ matrix.path }} directory
     runs-on: ubuntu-latest
     strategy:
       matrix:
@@ -22,5 +22,5 @@ jobs:
     - name: Run clang-format style check for C/C++/Protobuf programs.
       uses: jidicula/clang-format-action@v4.9.0
       with:
-        clang-format-version: '14'
-        check-path: ${{ matrix.path }}
+        clang-format-version: '16'
+        check-path: ${{ github.workspace }}/${{ matrix.path }}

CMakeLists.txt

@@ -44,10 +44,6 @@ if(BUILD_TESTS)
   enable_testing()
 endif()
 
-if(ENABLE_LOG)
-  add_compile_options(-DSPDLOG)
-endif()
-
 find_package(spdlog REQUIRED)
 message("Found package: spdlog")
 
@@ -78,8 +74,17 @@ foreach(TARGET ${TARGETS})
   if(ENABLE_WERROR)
     target_compile_options(${TARGET} PRIVATE -Werror)
   endif()
+
+  if(ENABLE_LOG)
+    target_compile_definitions(${TARGET} PUBLIC -DSPDLOG=1)
+  endif()
 endforeach()
 
 foreach(TOOL ${TOOLLIST})
   target_link_libraries(${TOOL} PRIVATE ${LIBLIST})
+  if(ENABLE_LOG)
+    target_compile_definitions(${TOOL} PRIVATE SPDLOG=1)
+  endif()
 endforeach()
+
+add_compile_options(-g3 -O3)

include/common/common.hh

@@ -12,7 +12,7 @@ static_assert(std::endian::little == std::endian::native,
               "It seems that u r trying to run our sim on ur router");
 
 static_assert(
-    -1 == ~0,
+    -1U == ~0U,
     "Two's complement representation is required. It is fixed since c++20");
 
 namespace sim {
@@ -68,6 +68,18 @@ template <typename T> consteval std::size_t sizeofBits() {
   return sizeof(T) * kBitsInByte;
 }
 
+template <std::size_t high, std::size_t low, std::unsigned_integral T = Word>
+constexpr T getBitsNoShift(T word) {
+  static_assert(high >= low, "Incorrect bits range");
+  static_assert(high < sizeofBits<T>(), "Bit index out of range");
+
+  auto mask = ~T(0);
+  if constexpr (high != sizeofBits<T>() - 1)
+    mask = ~(mask << (high + 1));
+
+  return static_cast<T>(word & mask);
+}
+
 /**
  * @brief Get bits from number function
  *
@@ -78,14 +90,7 @@ template <typename T> consteval std::size_t sizeofBits() {
  */
 template <std::size_t high, std::size_t low, std::unsigned_integral T = Word>
 constexpr T getBits(T word) {
-  static_assert(high >= low, "Incorrect bits range");
-  static_assert(high < sizeofBits<T>(), "Bit index out of range");
-
-  auto mask = ~T(0);
-  if constexpr (high != sizeofBits<T>() - 1)
-    mask = ~(mask << (high + 1));
-
-  return static_cast<T>((word & mask) >> low);
+  return static_cast<T>(getBitsNoShift<high, low>(word) >> low);
 }
 
 /**
@@ -99,7 +104,7 @@ constexpr T getBits(T word) {
 template <std::size_t pos, bool toSet> constexpr Word setBit(Word word) {
   static_assert(pos < sizeofBits<Word>(), "Bit index out of range");
 
-  constexpr auto mask = Word(1) << pos;
+  constexpr auto mask = static_cast<Word>(1) << pos;
   if constexpr (toSet)
     return word | mask;
 
@@ -145,7 +150,7 @@ constexpr Word signExtend(Word word) {
     return word;
 
   Word zeroed = getBits<oldSize - 1, 0>(word);
-  constexpr Word mask = Word(1) << (oldSize - 1);
+  constexpr Word mask = static_cast<Word>(1) << (oldSize - 1);
   Word res = (zeroed ^ mask) - mask;
 
   return getBits<newSize - 1, 0>(res);

include/common/counters.hh

@@ -42,15 +42,15 @@ public:
         {OpType::CSRRC, 1},   {OpType::CSRRWI, 1}, {OpType::CSRRSI, 1},
         {OpType::CSRRCI, 1},
     };
-    auto it = throughput.find(type);
-    if (it == throughput.end()) {
+    auto iter = throughput.find(type);
+    if (iter == throughput.end()) {
 #ifdef SPDLOG
       spdlog::warn("Cant calculate {} throughput",
                    static_cast<std::uint8_t>(type));
 #endif
       return 1;
     }
-    return it->second;
+    return iter->second;
   }
 }; // class Counters
 

include/common/inst.hh

@@ -12,6 +12,7 @@
 namespace sim {
 
 struct Instruction final {
+
   RegId rs1{};
   RegId rs2{};
   RegId rs3{};
@@ -25,9 +26,10 @@ struct Instruction final {
 
   bool isBranch{false};
 
-  std::string str() const;
+  [[nodiscard]] std::string str() const;
 
-  std::function<void(const Instruction &, State &)> callback = nullptr;
+  using Callback = void (*)(const Instruction &, State &);
+  Callback callback = nullptr;
 };
 
 using BasicBlock = std::vector<Instruction>;

include/common/state.hh

@@ -22,7 +22,7 @@ private:
   std::array<RegVal, kRegNum> regs{};
 
 public:
-  RegVal get(RegId regnum) const { return regs.at(regnum); }
+  [[nodiscard]] RegVal get(RegId regnum) const { return regs.at(regnum); }
 
   void set(RegId regnum, RegVal val) {
     // NOP instruction looks like ADD x0, x0, 0 - assignment to x0,
@@ -36,7 +36,7 @@ public:
     regs.at(regnum) = val;
   }
 
-  std::string str() const;
+  [[nodiscard]] std::string str() const;
 };
 
 class CSRegFile final {
@@ -46,7 +46,7 @@ private:
 public:
   CSRegFile() : regs(kCSRegNum) {}
 
-  RegVal get(CSRegId regnum) const { return regs.at(regnum); }
+  [[nodiscard]] RegVal get(CSRegId regnum) const { return regs.at(regnum); }
   void set(CSRegId regnum, RegVal val) { regs.at(regnum) = val; }
 
   void updateTimers(OpType type) {

include/common/timer.hh

@@ -13,14 +13,14 @@ public:
 
   void reset() { start = std::chrono::high_resolution_clock::now(); }
 
-  auto elapsedMs() {
+  [[nodiscard]] auto elapsedMs() {
     auto end = std::chrono::high_resolution_clock::now();
 
     return std::chrono::duration_cast<std::chrono::milliseconds>(end - start)
         .count();
   }
 
-  auto elapsedMcs() {
+  [[nodiscard]] auto elapsedMcs() {
     auto end = std::chrono::high_resolution_clock::now();
 
     return std::chrono::duration_cast<std::chrono::microseconds>(end - start)

include/elfloader/elfloader.hh

@@ -17,24 +17,25 @@ private:
   ELFIO::elfio elfFile_{};
 
 public:
-  ELFLoader(const fs::path &file);
-  ELFLoader(std::istream &stream);
+  explicit ELFLoader(const fs::path &file);
+  explicit ELFLoader(std::istream &stream);
 
-  Addr getEntryPoint() const;
+  [[nodiscard]] Addr getEntryPoint() const;
 
   using IndexT = unsigned;
-  std::vector<IndexT> getLoadableSegments() const;
+  [[nodiscard]] std::vector<IndexT> getLoadableSegments() const;
 
-  std::size_t getSegmentFileSize(IndexT index) const;
-  std::size_t getSegmentMemorySize(IndexT index) const;
-  std::span<const Word> getSegment(IndexT index) const;
-  Addr getSegmentAddr(IndexT index) const;
-  bool hasSegment(IndexT index) const;
+  [[nodiscard]] std::size_t getSegmentFileSize(IndexT index) const;
+  [[nodiscard]] std::size_t getSegmentMemorySize(IndexT index) const;
+  [[nodiscard]] std::span<const Word> getSegment(IndexT index) const;
+  [[nodiscard]] Addr getSegmentAddr(IndexT index) const;
+  [[nodiscard]] bool hasSegment(IndexT index) const;
 
 private:
   void check() const;
-  const ELFIO::section *getSectionPtr(const std::string &name) const;
-  const ELFIO::segment *getSegmentPtr(IndexT index) const;
+  [[nodiscard]] const ELFIO::section *
+  getSectionPtr(const std::string &name) const;
+  [[nodiscard]] const ELFIO::segment *getSegmentPtr(IndexT index) const;
 };
 
 } // namespace sim

include/executor/executor.hh

@@ -14,7 +14,8 @@
 namespace sim {
 
 template <typename T>
-concept InstForwardIterator = std::input_iterator<T> &&
+concept InstForwardIterator =
+    std::input_iterator<T> &&
     std::is_same_v<typename std::iterator_traits<T>::value_type, Instruction>;
 
 class Executor final {
@@ -24,6 +25,7 @@ public:
   Executor(Executor &&) = delete;
   Executor &operator=(const Executor &) = delete;
   Executor &operator=(Executor &&) = delete;
+  ~Executor() = default;
 
   void execute(const Instruction &inst, State &state) {
     inst.callback(inst, state);
@@ -49,11 +51,11 @@ public:
       spdlog::trace("Current regfile state:\n{}", state.regs.str());
 #endif
       this->instrCount += 1;
-      state.csregs.updateTimers(inst.type);
+      // state.csregs.updateTimers(inst.type);
     });
   }
 
-  std::uint64_t getInstrCount() const { return instrCount; }
+  [[nodiscard]] std::uint64_t getInstrCount() const { return instrCount; }
 
 private:
   std::uint64_t instrCount{1};

include/hart/hart.hh

@@ -19,7 +19,7 @@ namespace fs = std::filesystem;
 
 class IBBCache {
 public:
-  virtual ~IBBCache() {}
+  virtual ~IBBCache() = default;
   virtual const BasicBlock &
   lookupUpdate(Addr key, std::function<BasicBlock(Addr)> slowGetData) = 0;
 };
@@ -39,7 +39,9 @@ private:
 public:
   Hart(const fs::path &executable, std::int64_t bbCacheSize);
   void run();
-  std::uint64_t getInstrCount() const { return exec_.getInstrCount(); }
+  [[nodiscard]] std::uint64_t getInstrCount() const {
+    return exec_.getInstrCount();
+  }
 };
 
 } // namespace sim

include/memory/memory.hh

@@ -16,7 +16,7 @@
 namespace sim {
 
 struct Page {
-  Page() { wordStorage.resize(kPageSize / sizeof(Word)); }
+  Page() : wordStorage(kPageSize / sizeof(Word)) {}
 
   std::vector<Word> wordStorage{};
 };
@@ -53,11 +53,11 @@ public:
     TLBStats() = default;
   };
 
-  TLB() { tlb.resize(kTLBSize); }
+  TLB() : tlb(kTLBSize) {}
   PagePtr tlbLookup(Addr addr);
   void tlbUpdate(Addr addr, PagePtr page);
   TLBIndex getTLBIndex(Addr addr);
-  const TLBStats &getTLBStats() const;
+  [[nodiscard]] const TLBStats &getTLBStats() const;
   void tlbFlush();
 
 private:
@@ -76,7 +76,7 @@ public:
     uint16_t offset{};
 
     AddrSections(uint32_t pt, uint16_t off) : indexPt(pt), offset(off) {}
-    AddrSections(Addr addr) {
+    explicit AddrSections(Addr addr) {
       constexpr std::pair<uint8_t, uint8_t> of_bits{kOffsetBits - 1, 0};
       constexpr std::pair<uint8_t, uint8_t> pt_bits{sizeofBits<Addr>() - 1,
                                                     kOffsetBits};
@@ -89,12 +89,13 @@ public:
 
   class PageFaultException : public std::runtime_error {
   public:
-    PageFaultException(const char *msg) : std::runtime_error(msg) {}
+    explicit PageFaultException(const char *msg) : std::runtime_error(msg) {}
   };
 
   class MisAlignedAddrException : public std::runtime_error {
   public:
-    MisAlignedAddrException(const char *msg) : std::runtime_error(msg) {}
+    explicit MisAlignedAddrException(const char *msg)
+        : std::runtime_error(msg) {}
   };
 
   enum struct MemoryOp { STORE = 0, LOAD = 1 };
@@ -106,7 +107,7 @@ public:
   template <isSimType T, PhysMemory::MemoryOp op> T *getEntity(Addr addr);
   uint16_t getOffset(Addr addr);
 
-  const TLB::TLBStats getTLBStats() { return tlb.getTLBStats(); }
+  [[nodiscard]] const TLB::TLBStats getTLBStats() { return tlb.getTLBStats(); }
 
 private:
   PT pageTable{};
@@ -133,6 +134,7 @@ public:
   Memory(Memory &&) = delete;
   Memory &operator=(const Memory &) = delete;
   Memory &operator=(Memory &&) = delete;
+  ~Memory() = default;
 
   Word loadWord(Addr addr);
   void storeWord(Addr addr, Word word);
@@ -145,12 +147,12 @@ public:
   void setProgramStoredFlag() { isProgramStored = true; }
 
   void printMemStats(std::ostream &ost) const;
-  const MemoryStats &getMemStats() const;
+  [[nodiscard]] const MemoryStats &getMemStats() const;
 
   template <std::forward_iterator It>
   void storeRange(Addr start, It begin, It end);
 
-  const TLB::TLBStats getTLBStats() { return physMem.getTLBStats(); }
+  [[nodiscard]] TLB::TLBStats getTLBStats() { return physMem.getTLBStats(); }
 };
 
 template <isSimType T, PhysMemory::MemoryOp op>
@@ -162,13 +164,17 @@ inline T *PhysMemory::getEntity(Addr addr) {
 #endif
   AddrSections sections(addr);
   auto offset = sections.offset;
-  auto isInTLB = tlb.tlbLookup(addr);
-  PagePtr page;
+  auto addrZerored =
+      getBitsNoShift<sizeofBits<decltype(addr)>() - 1, kOffsetBits>(addr);
+
+  auto isInTLB = tlb.tlbLookup(addrZerored);
+  PagePtr page{};
+
   if (isInTLB) {
     page = isInTLB;
   } else {
     page = PhysMemory::pageTableLookup<op>(sections);
-    tlb.tlbUpdate(addr, page);
+    tlb.tlbUpdate(addrZerored, page);
   }
   Word *word = &page->wordStorage.at(offset / sizeof(Word));
   Byte *byte = reinterpret_cast<Byte *>(word) + (offset % sizeof(Word));