Cleans up CMake and fixes some warnings/bugs

- Also fixes a few bugs
- Fixes some TSAN findings

CMakeLists.txt

@@ -22,6 +22,8 @@ option(LLVMLIB_FRONTEND "Enables the front-end using the LLVM library to read LL
 option(ENABLE_COVERAGE "Enables collection of code coverage via gcov" OFF)
 # Option whether to enable use of the CLang library (EXPERIMENTAL)
 option(CLANG_LIBRARY "Uses the libclang library for compilation, uses the clang executable otherwise" OFF)
+# Option whether to enable more compile-time checks
+option(ADVANCED_CHECKS "Enable advanced compile-time checks" OFF)
 
 # Path to the VC4CL standard library
 # NOTE: Resolving ~ (for home directory) is currently not supported
@@ -78,7 +80,7 @@ endif()
 
 # clang-tidy
 find_program(CLANG_TIDY NAMES clang-tidy clang-tidy-5.0 clang-tidy-6.0 clang-tidy-7 clang-tidy-8)
-if(FALSE AND BUILD_DEBUG AND CLANG_TIDY)
+if(ADVANCED_CHECKS AND CLANG_TIDY)
     message(STATUS "Enabling clang-tidy compile time checks: ${CLANG_TIDY}")
     set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY}")
 endif()

cmake/sanitizers.cmake

@@ -14,23 +14,30 @@ elseif("${CMAKE_BUILD_TYPE}" STREQUAL "ubsan")
   message(STATUS "Build will have UBSAN enabled!")
 endif()
 
+if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+  set(SANITIZERS_ADDITIONAL_FLAGS "-Og")
+else()
+  # -Og is not supported by CLang
+  set(SANITIZERS_ADDITIONAL_FLAGS "")
+endif()
+
 # AddressSanitize
-set(CMAKE_C_FLAGS_ASAN "-fsanitize=address -fno-omit-frame-pointer -g -Og")
-set(CMAKE_CXX_FLAGS_ASAN "-fsanitize=address -fno-omit-frame-pointer -g -Og")
+set(CMAKE_C_FLAGS_ASAN "-fsanitize=address -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
+set(CMAKE_CXX_FLAGS_ASAN "-fsanitize=address -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
 
 # ThreadSanitizer
-set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -fno-omit-frame-pointer -g -Og")
-set(CMAKE_CXX_FLAGS_TSAN "-fsanitize=thread -fno-omit-frame-pointer -g -Og")
+set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
+set(CMAKE_CXX_FLAGS_TSAN "-fsanitize=thread -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
 
 # LeakSanitizer
-set(CMAKE_C_FLAGS_LSAN "-fsanitize=leak -fno-omit-frame-pointer -g -Og")
-set(CMAKE_CXX_FLAGS_LSAN "-fsanitize=leak -fno-omit-frame-pointer -g -Og")
+set(CMAKE_C_FLAGS_LSAN "-fsanitize=leak -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
+set(CMAKE_CXX_FLAGS_LSAN "-fsanitize=leak -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
 
 # MemorySanitizer (Clang only!)
 set(CMAKE_C_FLAGS_MSAN "-fsanitize=memory -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O2")
 set(CMAKE_CXX_FLAGS_MSAN "-fsanitize=memory -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O2")
 
 # UndefinedBehaviour
 # Run with: UBSAN_OPTIONS=print_stacktrace=1 environment variable
-set(CMAKE_C_FLAGS_UBSAN "-fsanitize=undefined -fsanitize=float-divide-by-zero -fsanitize=float-cast-overflow -fno-omit-frame-pointer -g -Og")
-set(CMAKE_CXX_FLAGS_UBSAN "-fsanitize=undefined -fsanitize=float-divide-by-zero -fsanitize=float-cast-overflow -fno-omit-frame-pointer -g -Og")
+set(CMAKE_C_FLAGS_UBSAN "-fsanitize=undefined -fsanitize=float-divide-by-zero -fsanitize=float-cast-overflow -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")
+set(CMAKE_CXX_FLAGS_UBSAN "-fsanitize=undefined -fsanitize=float-divide-by-zero -fsanitize=float-cast-overflow -fno-omit-frame-pointer -g ${SANITIZERS_ADDITIONAL_FLAGS}")

src/Bitfield.h

@@ -19,6 +19,11 @@
 
 namespace vc4c
 {
+    template <typename T>
+    using underlying_type = typename std::conditional<std::is_enum<T>::value, std::underlying_type<T>,
+        typename std::conditional<std::is_integral<T>::value || std::is_same<T, bool>::value, std::remove_reference<T>,
+            T>::type>::type;
+
     /*
      * A bit-field is an integral type with a specific binary layout where different parts of the binary code have
      * different meanings.
@@ -102,7 +107,8 @@ namespace vc4c
         template <typename T>
         constexpr inline T getEntry(UnderlyingType pos, UnderlyingType mask) const noexcept
         {
-            return static_cast<T>(mask & getValue(pos));
+            auto tmp = static_cast<typename underlying_type<T>::type>(mask & getValue(pos));
+            return static_cast<T>(tmp);
         }
 
         template <typename T>
@@ -193,6 +199,7 @@ namespace vc4c
      */
     struct InstructionPart
     {
+        using type = unsigned char;
         unsigned char value;
 
         explicit constexpr InstructionPart(unsigned char val) noexcept : value(val) {}

src/Compiler.cpp

@@ -108,8 +108,9 @@ std::size_t Compiler::convert()
     norm.adjust(module);
     PROFILE_END(SecondNormalizer);
 
+    auto kernels = module.getKernels();
     const auto f = [&codeGen](Method* kernelFunc) -> void { codeGen.toMachineCode(*kernelFunc); };
-    ThreadPool{"CodeGenerator"}.scheduleAll<Method*>(module.getKernels(), f);
+    ThreadPool{"CodeGenerator"}.scheduleAll<Method*>(kernels, f);
 
     // TODO could discard unused globals
     // since they are exported, they are still in the intermediate code, even if not used (e.g. optimized away)

src/Types.cpp

@@ -449,7 +449,12 @@ bool StructType::operator==(const ComplexType& other) const
     const StructType* right = dynamic_cast<const StructType*>(&other);
     if(right == nullptr)
         return false;
-    return isPacked == right->isPacked && elementTypes == right->elementTypes;
+    // NOTE: comparing by name is technically not necessary, since we don't actually care whether two struct types with
+    // the same layout are the exact same types or not. But it is required to work around a quirk in
+    // BitcodeReader#toDataType, since there struct types are inserted empty into the TypeHolder and after that the
+    // element types are resolved. If now the first element type is also a newly created struct type, both types have no
+    // elements and are therefore considered equal. Occurs e.g. in testing/bullet/jointSolver.cl.
+    return name == right->name && isPacked == right->isPacked && elementTypes == right->elementTypes;
 }
 
 unsigned int StructType::getStructSize(const int index) const

src/Units.h

@@ -17,6 +17,7 @@ namespace vc4c
     struct Byte
     {
     public:
+        using type = uint64_t;
         constexpr explicit Byte(uint64_t val) : value(val) {}
 
         constexpr explicit operator uint64_t() const
@@ -65,6 +66,7 @@ namespace vc4c
     struct Word
     {
     public:
+        using type = uint64_t;
         constexpr explicit Word(uint64_t val) : value(val) {}
         constexpr explicit Word(Byte bytes) : value(bytes.value / sizeof(uint64_t)) {}
 

src/Values.cpp

@@ -540,7 +540,7 @@ SIMDVector SIMDVector::rotate(uint8_t offset) const &
     SIMDVector copy(*this);
     //"Rotates the order of the elements in the range [first,last), in such a way that the element pointed by middle
     // becomes the new first element."
-    offset = (NATIVE_VECTOR_SIZE - offset);
+    offset = static_cast<uint8_t>(NATIVE_VECTOR_SIZE - offset);
     std::rotate(copy.begin(), copy.begin() + offset, copy.end());
     return copy;
 }
@@ -549,7 +549,7 @@ SIMDVector SIMDVector::rotate(uint8_t offset) &&
 {
     //"Rotates the order of the elements in the range [first,last), in such a way that the element pointed by middle
     // becomes the new first element."
-    offset = (NATIVE_VECTOR_SIZE - offset);
+    offset = static_cast<uint8_t>(NATIVE_VECTOR_SIZE - offset);
     std::rotate(begin(), begin() + offset, end());
     return std::move(*this);
 }

src/asm/OpCodes.cpp

@@ -754,7 +754,7 @@ static PrecalculatedLiteral calcLiteral(const OpCode& code, Literal firstLit, Li
         // Tests have shown that on VC4 all shifts (asr, shr, shl) only take the last 5 bits of the offset (modulo 32)
         auto offset = secondLit.unsignedInt() & 0x1F;
         // carry is set if bits set are shifted out of the register: val & (2^shift-offset-1) != 0
-        auto shiftLoss = firstLit.unsignedInt() & ((1 << offset) - 1);
+        auto shiftLoss = firstLit.unsignedInt() & ((1u << offset) - 1u);
         return setFlags(intermediate::asr(firstLit, secondLit), shiftLoss != 0, false);
     }
     if(code == OP_CLZ)
@@ -851,7 +851,7 @@ static PrecalculatedLiteral calcLiteral(const OpCode& code, Literal firstLit, Li
         // Tests have shown that on VC4 all shifts (asr, shr, shl) only take the last 5 bits of the offset (modulo 32)
         auto offset = secondLit.unsignedInt() & 0x1F;
         // carry is set if bits set are shifted out of the register: val & (2^shift-offset-1) != 0
-        auto shiftLoss = firstLit.unsignedInt() & ((1 << offset) - 1);
+        auto shiftLoss = firstLit.unsignedInt() & ((1u << offset) - 1u);
         return setFlags(Literal(firstLit.unsignedInt() >> offset), shiftLoss != 0);
     }
     if(code == OP_SUB)

src/helper.h

@@ -21,7 +21,7 @@
 
 #if __cplusplus > 201402L
 #define FALL_THROUGH [[fallthrough]];
-#elif defined(__clang_major__) && __clang_major__ >= 4
+#elif defined(__clang_major__) && (__clang_major__ >= 4 || (__clang_major__ == 3 && __clang_minor__ == 9))
 #define FALL_THROUGH [[clang::fallthrough]];
 #elif defined(__GNUC__) && __GNUC__ >= 7
 #define FALL_THROUGH __attribute__((fallthrough));

src/intermediate/VectorHelper.cpp

@@ -172,7 +172,7 @@ InstructionWalker intermediate::insertVectorInsertion(
         // other unchanged
         // we use the mask version of loads to set the elements we want to insert to and then use flags to insert only
         // those
-        unsigned maskLit = (1 << value.type.getVectorWidth()) - 1;
+        unsigned maskLit = (1u << value.type.getVectorWidth()) - 1u;
         auto shiftedMask = method.addNewLocal(TYPE_INT32, "%vector_mask");
         if(auto lit = index.getLiteralValue())
         {
@@ -392,7 +392,8 @@ InstructionWalker intermediate::insertVectorShuffle(InstructionWalker it, Method
         else
         {
             it = insertVectorConcatenation(it, method, source0, source1, destination);
-            numCorrespondingIndices = source0.type.getVectorWidth() + source1.type.getVectorWidth();
+            numCorrespondingIndices =
+                static_cast<uint8_t>(source0.type.getVectorWidth() + source1.type.getVectorWidth());
         }
     }
 
@@ -413,7 +414,7 @@ InstructionWalker intermediate::insertVectorShuffle(InstructionWalker it, Method
             std::make_pair(rotateElementNumberDown(maskContainer[i].unsignedInt() - firstVectorSize, i), 1) :
             // source is from first vector
             std::make_pair(rotateElementNumberDown(maskContainer[i].unsignedInt(), i), 0);
-        relativeSources[source] |= (1 << i);
+        relativeSources[source] |= (1u << i);
     }
 
     for(const auto& sources : relativeSources)

src/intermediate/operators.h

@@ -432,6 +432,12 @@ namespace vc4c
                 it.nextInBlock();
             }
 
+            void operator=(Value&& src) &&
+            {
+                it.emplace(new intermediate::MoveOperation(Value(result), std::move(src)));
+                it.nextInBlock();
+            }
+
             NODISCARD ConditionCode operator=(ComparisonWrapper&& op) &&
             {
                 return op(it, result, op.arg0, op.arg1);
@@ -483,6 +489,14 @@ namespace vc4c
                 it.nextInBlock();
                 return result;
             }
+
+            NODISCARD Value operator=(Value&& src) &&
+            {
+                auto result = method.addNewLocal(type, name);
+                it.emplace(new intermediate::MoveOperation(Value(result), std::move(src)));
+                it.nextInBlock();
+                return result;
+            }
         };
 
         /**

src/intrinsics/Images.cpp

@@ -320,5 +320,4 @@ InstructionWalker intermediate::insertQueryMeasurements(
     else
         throw CompilationError(CompilationStep::GENERAL, "Unsupported image dimensions",
             std::to_string(static_cast<unsigned>(imageType->dimensions)));
-    throw CompilationError(CompilationStep::GENERAL, "Unimplemented image-query function", it->to_string());
 }

src/intrinsics/Intrinsics.cpp

@@ -644,7 +644,8 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
                           op->conditional, op->setFlags))
                          ->copyExtrasFrom(it.get()));
         }
-        else if(arg0.getLiteralValue() && isPowerTwo(arg0.getLiteralValue()->signedInt()))
+        else if(arg0.getLiteralValue() && arg0.getLiteralValue()->signedInt() > 0 &&
+            isPowerTwo(arg0.getLiteralValue()->unsignedInt()))
         {
             // a * 2^n = a << n
             CPPLOG_LAZY(logging::Level::DEBUG,
@@ -655,7 +656,8 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
                      op->conditional, op->setFlags))
                     ->copyExtrasFrom(it.get()));
         }
-        else if(arg1.getLiteralValue() && isPowerTwo(arg1.getLiteralValue()->signedInt()))
+        else if(arg1.getLiteralValue() && arg1.getLiteralValue()->signedInt() > 0 &&
+            isPowerTwo(arg1.getLiteralValue()->unsignedInt()))
         {
             // a * 2^n = a << n
             CPPLOG_LAZY(logging::Level::DEBUG,
@@ -674,7 +676,8 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
                           op->conditional, op->setFlags))
                          ->copyExtrasFrom(it.get()));
         }
-        else if(arg0.getLiteralValue() && isPowerTwo(arg0.getLiteralValue()->signedInt() + 1))
+        else if(arg0.getLiteralValue() && arg0.getLiteralValue()->signedInt() > 0 &&
+            isPowerTwo(arg0.getLiteralValue()->unsignedInt() + 1))
         {
             // x * (2^k - 1) = x * 2^k - x = x << k - x
             CPPLOG_LAZY(logging::Level::DEBUG,
@@ -686,7 +689,8 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
             it.reset((new Operation(OP_SUB, op->getOutput().value(), tmp, arg1, op->conditional, op->setFlags))
                          ->copyExtrasFrom(it.get()));
         }
-        else if(arg1.getLiteralValue() && isPowerTwo(arg1.getLiteralValue()->signedInt() + 1))
+        else if(arg1.getLiteralValue() && arg1.getLiteralValue()->signedInt() > 0 &&
+            isPowerTwo(arg1.getLiteralValue()->unsignedInt() + 1))
         {
             // x * (2^k - 1) = x * 2^k - x = x << k - x
             CPPLOG_LAZY(logging::Level::DEBUG,
@@ -734,7 +738,8 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
             it->addDecorations(InstructionDecorations::UNSIGNED_RESULT);
         }
         // a / 2^n = a >> n
-        else if(arg1.getLiteralValue() && isPowerTwo(arg1.getLiteralValue()->signedInt()))
+        else if(arg1.getLiteralValue() && arg1.getLiteralValue()->signedInt() > 0 &&
+            isPowerTwo(arg1.getLiteralValue()->unsignedInt()))
         {
             CPPLOG_LAZY(logging::Level::DEBUG,
                 log << "Intrinsifying division with right-shift: " << op->to_string() << logging::endl);
@@ -772,7 +777,7 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
         }
         // a / 2^n = (abs(a) >> n) * sign(a)
         else if(arg1.isLiteralValue() && arg1.getLiteralValue()->signedInt() > 0 &&
-            isPowerTwo(arg1.getLiteralValue()->signedInt()))
+            isPowerTwo(arg1.getLiteralValue()->unsignedInt()))
         {
             CPPLOG_LAZY(logging::Level::DEBUG,
                 log << "Intrinsifying signed division with right-shift and sign-copy: " << op->to_string()
@@ -874,7 +879,7 @@ static NODISCARD InstructionWalker intrinsifyArithmetic(Method& method, Instruct
         }
         // a % 2^n = (abs(a) & 2^n-1) * sign(a)
         else if(arg1.isLiteralValue() && arg1.getLiteralValue()->signedInt() > 0 &&
-            isPowerTwo(arg1.getLiteralValue()->signedInt()))
+            isPowerTwo(arg1.getLiteralValue()->unsignedInt()))
         {
             CPPLOG_LAZY(logging::Level::DEBUG,
                 log << "Intrinsifying signed modulo by power of two: " << op->to_string() << logging::endl);

src/llvm/BitcodeReader.cpp

@@ -690,10 +690,10 @@ Method& BitcodeReader::parseFunction(Module& module, const llvm::Function& func)
             toParameterDecorations(arg, type, func.getCallingConv() == llvm::CallingConv::SPIR_KERNEL)));
         auto& param = method->parameters.back();
         if(arg.getDereferenceableBytes() != 0)
-            param.maxByteOffset = arg.getDereferenceableBytes();
+            param.maxByteOffset = static_cast<std::size_t>(arg.getDereferenceableBytes());
 #if LLVM_LIBRARY_VERSION >= 39
         if(arg.getDereferenceableOrNullBytes() != 0)
-            param.maxByteOffset = arg.getDereferenceableOrNullBytes();
+            param.maxByteOffset = static_cast<std::size_t>(arg.getDereferenceableOrNullBytes());
 #endif
         CPPLOG_LAZY(logging::Level::DEBUG, log << "Reading parameter " << param.to_string(true) << logging::endl);
         if(param.type.getImageType() && func.getCallingConv() == llvm::CallingConv::SPIR_KERNEL)

src/main.cpp

@@ -100,7 +100,7 @@ static void printHelp()
 static std::string toVersionString(unsigned version)
 {
     std::stringstream s;
-    s << (version / 10.0f);
+    s << (static_cast<float>(version) / 10.0f);
     return s.str();
 }
 

src/normalization/AddressCalculation.h

@@ -122,4 +122,4 @@ namespace vc4c
         };
     } /* namespace normalization */
 } /* namespace vc4c */
-#endif /* VC4C_NORMALIZATION_ADDRESS_CALCULATION_H */
+#endif /* VC4C_NORMALIZATION_ADDRESS_CALCULATION_H */

src/normalization/MemoryAccess.cpp

@@ -87,11 +87,12 @@ static BaseAndOffset findBaseAndOffset(const Value& val)
                 ->local()
                 ->getBase(false)
                 ->createReference(),
-            static_cast<int32_t>((*std::find_if(args.begin(), args.end(),
-                                      [](const Value& arg) -> bool { return arg.getLiteralValue().has_value(); }))
-                                     .getLiteralValue()
-                                     ->signedInt() /
-                val.type.getElementType().getInMemoryWidth()));
+            (*std::find_if(
+                 args.begin(), args.end(), [](const Value& arg) -> bool { return arg.getLiteralValue().has_value(); }))
+                    .getLiteralValue()
+                    ->signedInt() /
+                /* in-memory width can't be > 2^30 anyway */
+                static_cast<int32_t>(val.type.getElementType().getInMemoryWidth()));
     }
 
     // 3. an addition with two locals -> one is the base, the other the calculation of the literal
@@ -103,10 +104,10 @@ static BaseAndOffset findBaseAndOffset(const Value& val)
         const auto offset1 = findOffset(args[1]);
         if(offset0.offset && args[1].checkLocal())
             return BaseAndOffset(args[1].local()->getBase(false)->createReference(),
-                static_cast<int32_t>(offset0.offset.value() / val.type.getElementType().getInMemoryWidth()));
+                offset0.offset.value() / static_cast<int32_t>(val.type.getElementType().getInMemoryWidth()));
         if(offset1.offset && args[0].checkLocal())
             return BaseAndOffset(args[0].local()->getBase(false)->createReference(),
-                static_cast<int32_t>(offset1.offset.value() / val.type.getElementType().getInMemoryWidth()));
+                offset1.offset.value() / static_cast<int32_t>(val.type.getElementType().getInMemoryWidth()));
     }
     /*
         if(writers.size() == 1)

src/normalization/MemoryMapChecks.cpp

@@ -60,7 +60,7 @@ Optional<Value> normalization::getConstantValue(const Value& source)
         return globalContainer->at(0).toValue();
     if(globalContainer && source.local()->reference.second >= 0)
         // fixed index
-        return globalContainer->at(source.local()->reference.second).toValue();
+        return globalContainer->at(static_cast<std::size_t>(source.local()->reference.second)).toValue();
     if(auto val = global->initialValue.toValue())
     {
         if(auto vector = val->checkVector())