Warnings

.jenkins/build_arm22.sh

@@ -15,3 +15,7 @@ module load tools/cmake
 build armclang armclang++
 test
 run
+
+buildRelease armclang armclang++
+test
+run

.jenkins/build_gcc10.sh

@@ -15,3 +15,7 @@ module load tools/cmake
 build gcc g++
 test
 run
+
+buildRelease gcc g++
+test
+run

.jenkins/build_gcc7.sh

@@ -15,3 +15,7 @@ module load tools/cmake
 build gcc g++
 test
 run
+
+buildRelease gcc g++
+test
+run

.jenkins/build_gcc8.sh

@@ -15,3 +15,7 @@ module load tools/cmake
 build gcc g++
 test
 run
+
+buildRelease gcc g++
+test
+run

.jenkins/build_gcc9.sh

@@ -15,3 +15,7 @@ module load tools/cmake
 build gcc g++
 test
 run
+
+buildRelease gcc g++
+test
+run

.jenkins/build_test_run.sh

@@ -36,6 +36,16 @@ build () {
     cmake --build build --target install
 }
 
+# Build common function
+buildRelease () {
+    cd "$SIMENG_TOP" || exit
+    rm -rf build/* install/*
+
+    cmake -B build -S . -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="$SIMENG_INSTALL" -DSIMENG_ENABLE_TESTS=ON -DSIMENG_USE_EXTERNAL_LLVM=ON -DLLVM_DIR=/home/br-simeng/llvm14.0.5/install-gcc7/lib/cmake/llvm/ -DCMAKE_C_COMPILER=$1 -DCMAKE_CXX_COMPILER=$2
+    cmake --build build -j
+    cmake --build build --target install
+}
+
 # Run tests
 test () {
     cd "$SIMENG_BUILD" || exit
@@ -52,16 +62,16 @@ run () {
     echo "Simulation without configuration file argument:"
     cat run
     echo ""
-    compare_outputs "$(grep "retired:" run | rev | cut -d ' ' -f1 | rev)" "3145731" "retired instructions"
-    compare_outputs "$(grep "cycles:" run | rev | cut -d ' ' -f1 | rev)" "3145732" "simulated cycles"
+    compare_outputs "$(grep "retired:" run | rev | cut -d ' ' -f1 | rev)" "6708" "retired instructions"
+    compare_outputs "$(grep "cycles:" run | rev | cut -d ' ' -f1 | rev)" "7955" "simulated cycles"
     echo ""
 
     ./bin/simeng "$SIMENG_TOP"/configs/tx2.yaml > run
     echo "Simulation with configuration file argument:"
     cat run
     echo ""
-    compare_outputs "$(grep "retired:" run | rev | cut -d ' ' -f1 | rev)" "3145732" "retired instructions"
-    compare_outputs "$(grep "cycles:" run | rev | cut -d ' ' -f1 | rev)" "1048588" "simulated cycles"
+    compare_outputs "$(grep "retired:" run | rev | cut -d ' ' -f1 | rev)" "6724" "retired instructions"
+    compare_outputs "$(grep "cycles:" run | rev | cut -d ' ' -f1 | rev)" "8677" "simulated cycles"
     echo ""
 }
 

CMakeLists.txt

@@ -71,8 +71,8 @@ set(CMAKE_MACOSX_RPATH 1)
 # Enable PIC for libraries
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 
-# Enable additional compiler warnings for all targets
-add_compile_options(-Wall)
+# Create variable to enable additional compiler warnings for SimEng targets only
+set(SIMENG_COMPILE_OPTIONS -Wall -pedantic -Werror) #-Wextra
 
 # Disable RTTI for all targets
 add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-fno-rtti>)
@@ -183,6 +183,8 @@ if(SIMENG_ENABLE_TESTS)
     set(LLVM_ENABLE_BINDINGS OFF)
     set(LLVM_INSTALL_UTILS OFF)
 
+    set(LLVM_ENABLE_WARNINGS OFF)
+
     # XXX all LLVM specific cmake variables must be set BEFORE FetchContent_MakeAvailable otherwise they have no effect
     FetchContent_MakeAvailable_SubDir_Args(llvm llvm-14.0.5.src EXCLUDE_FROM_ALL)
     # make sure we get the headers too

src/include/simeng/CoreInstance.hh

@@ -21,19 +21,6 @@
 #include "simeng/pipeline/A64FXPortAllocator.hh"
 #include "simeng/pipeline/BalancedPortAllocator.hh"
 
-// Program used when no executable is provided; counts down from
-// 1024*1024, with an independent `orr` at the start of each branch.
-static uint32_t hex_[] = {
-    0x320C03E0,  // orr w0, wzr, #1048576
-    0x320003E1,  // orr w0, wzr, #1
-    0x71000400,  // subs w0, w0, #1
-    0x54FFFFC1,  // b.ne -8
-                 // .exit:
-    0xD2800000,  // mov x0, #0
-    0xD2800BC8,  // mov x8, #94
-    0xD4000001,  // svc #0
-};
-
 namespace simeng {
 
 /** A class to create a SimEng core instance from a supplied config. */
@@ -46,7 +33,7 @@ class CoreInstance {
 
   /** CoreInstance with source code assembled by LLVM and a model configuration.
    */
-  CoreInstance(char* assembledSource, size_t sourceSize,
+  CoreInstance(uint8_t* assembledSource, size_t sourceSize,
                ryml::ConstNodeRef config = config::SimInfo::getConfig());
 
   ~CoreInstance();
@@ -75,10 +62,10 @@ class CoreInstance {
   std::shared_ptr<char> getProcessImage() const;
 
   /** Getter for the size of the created process image. */
-  const uint64_t getProcessImageSize() const;
+  uint64_t getProcessImageSize() const;
 
   /* Getter for heap start. */
-  const uint64_t getHeapStart() const;
+  uint64_t getHeapStart() const;
 
  private:
   /** Generate the appropriate simulation objects as parameterised by the
@@ -111,7 +98,7 @@ class CoreInstance {
   simeng::kernel::Linux kernel_;
 
   /** Reference to source assembled by LLVM. */
-  char* source_ = nullptr;
+  uint8_t* source_ = nullptr;
 
   /** Size of the source code assembled by LLVM. */
   size_t sourceSize_ = 0;

src/include/simeng/RegisterValue.hh

@@ -54,7 +54,7 @@ class RegisterValue {
    */
   RegisterValue(const char* ptr, uint16_t bytes, uint16_t capacity)
       : bytes(capacity) {
-    assert(capacity >= bytes && "Capacity is less then requested bytes");
+    assert(capacity >= bytes && "Capacity is less than requested bytes");
     char* dest;
     if (isLocal()) {
       dest = this->value;

src/include/simeng/arch/Architecture.hh

@@ -52,7 +52,7 @@ class Architecture {
    * Writes into the supplied macro-op vector, and returns the number of bytes
    * consumed to produce it; a value of 0 indicates too few bytes were present
    * for a valid decoding. */
-  virtual uint8_t predecode(const void* ptr, uint16_t bytesAvailable,
+  virtual uint8_t predecode(const uint8_t* ptr, uint16_t bytesAvailable,
                             uint64_t instructionAddress,
                             MacroOp& output) const = 0;
 

src/include/simeng/arch/aarch64/ArchInfo.hh

@@ -37,9 +37,9 @@ class ArchInfo : public simeng::arch::ArchInfo {
     uint16_t predCount = regConfig["Predicate-Count"].as<uint16_t>();
     uint16_t condCount = regConfig["Conditional-Count"].as<uint16_t>();
     uint16_t matCount = regConfig["Matrix-Count"].as<uint16_t>();
-    // Matrix-Count multiplied by (SVL/8) as internal representation of
-    // ZA is a block of row-vector-registers. Therefore we need to
-    // convert physical counts from whole-ZA to rows-in-ZA.
+    // Matrix-Count multiplied by (SVL/8) as internal representation of ZA is a
+    // block of row-vector-registers. Therefore, we need to convert physical
+    // counts from whole-ZA to rows-in-ZA.
     matCount *= zaSize_;
     physRegStruct_ = {{8, gpCount},
                       {256, fpCount},

src/include/simeng/arch/aarch64/Architecture.hh

@@ -25,7 +25,7 @@ class Architecture : public arch::Architecture {
   /** Pre-decode instruction memory into a macro-op of `Instruction`
    * instances. Returns the number of bytes consumed to produce it (always 4),
    * and writes into the supplied macro-op vector. */
-  uint8_t predecode(const void* ptr, uint16_t bytesAvailable,
+  uint8_t predecode(const uint8_t* ptr, uint16_t bytesAvailable,
                     uint64_t instructionAddress,
                     MacroOp& output) const override;
 

src/include/simeng/arch/aarch64/helpers/float.hh

@@ -157,6 +157,12 @@ RegisterValue scvtf_FixedPoint(
  * Returns single value of type D. */
 template <typename D, typename N>
 D fcvtzu_integer(srcValContainer& sourceValues) {
+  // Ensure types so that we know behaviour of inaccurate type conversions
+  static_assert((std::is_same<float, N>() || std::is_same<double, N>()) &&
+                "N is not a valid type which should be float or double");
+  static_assert((std::is_same<uint32_t, D>() || std::is_same<uint64_t, D>()) &&
+                "D is not a valid type which should be int32_t or int64_t");
+
   N input = sourceValues[0].get<N>();
   D result = static_cast<D>(0);
 
@@ -165,7 +171,18 @@ D fcvtzu_integer(srcValContainer& sourceValues) {
     if (std::isinf(input)) {
       // Account for Infinity
       result = std::numeric_limits<D>::max();
-    } else if (input > std::numeric_limits<D>::max()) {
+    } else if (static_cast<double>(input) >=
+               static_cast<double>(std::numeric_limits<D>::max())) {
+      // Cast to double to ensure no precision errors. Float can't store uint32
+      // or uint64 max values accurately as not enough bits available. This
+      // causes unwanted comparison behaviour
+      //
+      // max() will be either 4294967295 or 18446744073709551615
+      // Casting to float results in the following (incorrect) values 4294967296
+      // (+1) or 18446744073709551616 (+1)
+      //
+      // Casting to double results in no erroneous conversion.
+
       // Account for the source value being larger than the
       // destination register can support
       result = std::numeric_limits<D>::max();

src/include/simeng/arch/aarch64/helpers/neon.hh

@@ -146,7 +146,7 @@ RegisterValue vecCountPerByte(srcValContainer& sourceValues) {
   const uint8_t* n = sourceValues[0].getAsVector<uint8_t>();
   T out[16 / sizeof(T)] = {0};
   for (int i = 0; i < I; i++) {
-    for (int j = 0; j < (sizeof(T) * 8); j++) {
+    for (size_t j = 0; j < (sizeof(T) * 8); j++) {
       // Move queried bit to LSB and extract via an AND operator
       out[i] += ((n[i] >> j) & 1);
     }
@@ -187,10 +187,10 @@ RegisterValue vecExtVecs_index(
   const uint64_t index = static_cast<uint64_t>(metadata.operands[3].imm);
   T out[16 / sizeof(T)] = {0};
 
-  for (int i = index; i < I; i++) {
+  for (uint64_t i = index; i < I; i++) {
     out[i - index] = n[i];
   }
-  for (int i = 0; i < index; i++) {
+  for (uint64_t i = 0; i < index; i++) {
     out[I - index + i] = m[i];
   }
   return {out, 256};
@@ -825,8 +825,8 @@ RegisterValue vecTbl(
   const uint8_t n_table_regs = metadata.operandCount - 2;
 
   // Create table from vectors. All table sourceValues must be of 16b format.
-  int tableSize = 16 * n_table_regs;
-  uint8_t table[tableSize];
+  const uint16_t tableSize = 16 * n_table_regs;
+  std::vector<uint8_t> table(tableSize, 0);
   for (int i = 0; i < n_table_regs; i++) {
     const int8_t* currentVector = sourceValues[i].getAsVector<int8_t>();
     for (int j = 0; j < 16; j++) {
@@ -836,7 +836,7 @@ RegisterValue vecTbl(
 
   int8_t out[16 / sizeof(int8_t)] = {0};
   for (int i = 0; i < I; i++) {
-    unsigned int index = Vm[i];
+    int8_t index = Vm[i];
 
     // If an index is out of range for the table, the result for that lookup
     // is 0

src/include/simeng/arch/aarch64/helpers/sve.hh

@@ -508,8 +508,8 @@ RegisterValue sveFcvtPredicated(srcValContainer& sourceValues,
 
   // Stores size of largest type out of D and N
   int lts = std::max(sizeof(D), sizeof(N));
-  bool sourceLarger = (sizeof(D) < sizeof(N)) ? true : false;
-  bool sameDandN = (sizeof(D) == sizeof(N)) ? true : false;
+  bool sourceLarger = (sizeof(D) < sizeof(N));
+  bool sameDandN = (sizeof(D) == sizeof(N));
 
   const uint16_t partition_num = VL_bits / (lts * 8);
   D out[256 / sizeof(D)] = {0};
@@ -543,14 +543,19 @@ RegisterValue sveFcvtPredicated(srcValContainer& sourceValues,
 template <typename D, typename N>
 RegisterValue sveFcvtzsPredicated(srcValContainer& sourceValues,
                                   const uint16_t VL_bits) {
+  static_assert((std::is_same<float, N>() || std::is_same<double, N>()) &&
+                "N is not a valid type which should be float or double");
+  static_assert((std::is_same<int32_t, D>() || std::is_same<int64_t, D>()) &&
+                "D is not a valid type which should be int32_t or int64_t");
+
   const D* d = sourceValues[0].getAsVector<D>();
   const uint64_t* p = sourceValues[1].getAsVector<uint64_t>();
   const N* n = sourceValues[2].getAsVector<N>();
 
   // Stores size of largest type out of D and N
   int lts = std::max(sizeof(D), sizeof(N));
-  bool sameType = (sizeof(D) == sizeof(N)) ? true : false;
-  bool sourceLarger = (sizeof(D) < sizeof(N)) ? true : false;
+  bool sameType = (sizeof(D) == sizeof(N));
+  bool sourceLarger = (sizeof(D) < sizeof(N));
 
   const uint16_t partition_num = VL_bits / (lts * 8);
   D out[256 / sizeof(D)] = {0};
@@ -561,7 +566,21 @@ RegisterValue sveFcvtzsPredicated(srcValContainer& sourceValues,
     int indexN = ((!sourceLarger) & (!sameType)) ? (2 * i) : i;
 
     if (p[i / (64 / lts)] & shifted_active) {
-      if (n[indexN] > std::numeric_limits<D>::max())
+      if (static_cast<double>(n[indexN]) >=
+          static_cast<double>(std::numeric_limits<D>::max()))
+        // Cast to double to reduce precision errors. Float can't store int32
+        // or int64 max values accurately as not enough bits available. This
+        // causes unwanted comparison behaviour. Double also can't accurately
+        // represent int64.MaxValue. Non-strict comparison used to capture this
+        // case
+        //
+        // max() will be either 2147483647 or 9223372036854775807
+        // Casting to float results in the following (incorrect) values
+        // 2147483648 (+1) or 9223372036854775808 (+1)
+        //
+        // Casting to double results in 2147483647 (+0) or incorrect
+        // 9223372036854775808(+1)
+
         out[indexOut] = std::numeric_limits<D>::max();
       else if (n[indexN] < std::numeric_limits<D>::lowest())
         out[indexOut] = std::numeric_limits<D>::lowest();

src/include/simeng/arch/riscv/ArchInfo.hh

@@ -25,7 +25,7 @@ const uint8_t addressAlignMask = 0x3;
 const uint8_t addressAlignMaskCompressed = 0x1;
 const uint8_t minInstWidthBytes = 4;
 const uint8_t minInstWidthBytesCompressed = 2;
-};  // namespace constantsPool
+}  // namespace constantsPool
 
 /** A class to hold and generate riscv specific architecture configuration
  * options. */

src/include/simeng/arch/riscv/Architecture.hh

@@ -24,7 +24,7 @@ class Architecture : public arch::Architecture {
   /** Pre-decode instruction memory into a macro-op of `Instruction`
    * instances. Returns the number of bytes consumed to produce it (0 if
    * failure), and writes into the supplied macro-op vector. */
-  uint8_t predecode(const void* ptr, uint16_t bytesAvailable,
+  uint8_t predecode(const uint8_t* ptr, uint16_t bytesAvailable,
                     uint64_t instructionAddress,
                     MacroOp& output) const override;
 

src/include/simeng/config/ExpectationNode.hh

@@ -235,7 +235,7 @@ class ExpectationNode {
     }
 
     definedSet_ = true;
-    for (const T s : set) {
+    for (const T& s : set) {
       DataTypeVariant dtv = s;
       expectedSet_.push_back(dtv);
     }
@@ -311,6 +311,11 @@ class ExpectationNode {
           }
           return {true, "Success"};
         }
+        default:
+          std::cerr << "[SimEng:validateConfigNode] Unexpected ExpectedType"
+                    << std::endl;
+
+          exit(-1);
       }
     }
   }
@@ -460,7 +465,8 @@ class ExpectationNode {
         }
       }
       if (!foundInSet) {
-        // Construct a human readable output denoted expected set failure
+        // Construct a human-readable output explaining failure to match the
+        // expected value set
         retStr << nodeVal << " not in set {";
         for (size_t i = 0; i < expectedSet_.size(); i++) {
           retStr << getByType<T>(expectedSet_[i]);
@@ -475,7 +481,8 @@ class ExpectationNode {
       // Check for value between bounds
       if (getByType<T>(expectedBounds_.first) > nodeVal ||
           getByType<T>(expectedBounds_.second) < nodeVal) {
-        // Construct a human readable output denoted expected bounds failure
+        // Construct a human-readable output explaining failure to be within
+        // expected bounds
         retStr << nodeVal << " not in the bounds {"
                << getByType<T>(expectedBounds_.first) << " to "
                << getByType<T>(expectedBounds_.second) << "}";
@@ -542,7 +549,9 @@ class ExpectationNode {
 
   /** The value bounds the associated config option is expected to lie between.
    */
-  std::pair<DataTypeVariant, DataTypeVariant> expectedBounds_;
+  // TODO needs initialisation in case validation called before setting. Unsure
+  // whether this is a good solution
+  std::pair<DataTypeVariant, DataTypeVariant> expectedBounds_ = {false, false};
 
   /** The instances of ExpectationNodes held within this node. Considered to be
    * the children of this node. */