fix(Tracing) Wrong data type in tracing

Fixed a bug during tracing of arithmetical operators containing operands with less than 32-bit. They were traced with there respective bit size and also the resulting val<> was of the respective bit size, but they were traced as 32-bit. We changed the `#define COMMON_RETURN_TYPE` to use the actual bit size. Signed-off-by: Nils Schubert <[email protected]>
nebulastream · Oct 8, 2024 · 6bd4061 · 6bd4061
1 parent 7adef5c
commit 6bd4061
Show file tree

Hide file tree

Showing 11 changed files with 71 additions and 16 deletions.
diff --git a/nautilus/include/nautilus/val.hpp b/nautilus/include/nautilus/val.hpp
@@ -18,10 +18,10 @@ template <typename LHS>
 LHS getRawValue(const val<LHS>& val);
 
 #define COMMON_RETURN_TYPE                                                                                             \
-	val<typename std::common_type<typename std::remove_cvref_t<LHS>::basic_type,                                       \
-	                              typename std::remove_cvref_t<RHS>::basic_type>::type>
+	typename std::common_type_t<typename std::remove_cvref_t<LHS>::basic_type,                                         \
+	                            typename std::remove_cvref_t<RHS>::basic_type>
 
-#define DEDUCT_RETURN_TYPE(OP) val<decltype(getRawValue(left) OP getRawValue(right))>
+#define DEDUCT_RETURN_TYPE(OP) decltype(getRawValue(left) OP getRawValue(right))
 
 template <typename T>
 tracing::TypedValueRef getState(T&& value) {
@@ -245,17 +245,17 @@ namespace details {
 #define DEFINE_BINARY_OPERATOR_HELPER(OP, OP_NAME, OP_TRACE, RES_TYPE)                                                 \
 	template <typename LHS, typename RHS>                                                                              \
 	auto inline OP_NAME(LHS&& left, RHS&& right) {                                                                     \
-		typedef typename std::common_type<typename std::remove_reference_t<LHS>::basic_type,                           \
-		                                  typename std::remove_reference_t<RHS>::basic_type>::type commonType;         \
+		typedef std::common_type_t<typename std::remove_reference_t<LHS>::basic_type,                                  \
+		                           typename std::remove_reference_t<RHS>::basic_type>                                  \
+		    commonType;                                                                                                \
 		auto&& lValue = cast_value<LHS, commonType>(std::forward<LHS>(left));                                          \
 		auto&& rValue = cast_value<RHS, commonType>(std::forward<RHS>(right));                                         \
-		using resultType = decltype(getRawValue(lValue) OP getRawValue(rValue));                                       \
 		if SHOULD_TRACE () {                                                                                           \
-			auto tc = tracing::traceBinaryOp(tracing::OP_TRACE, tracing::to_type<resultType>(),                        \
+			auto tc = tracing::traceBinaryOp(tracing::OP_TRACE, tracing::to_type<RES_TYPE>(),                          \
 			                                 details::getState(lValue), details::getState(rValue));                    \
-			return RES_TYPE(tc);                                                                                       \
+			return val<RES_TYPE>(tc);                                                                                  \
 		}                                                                                                              \
-		return RES_TYPE(getRawValue(lValue) OP getRawValue(rValue));                                                   \
+		return val<RES_TYPE>(getRawValue(lValue) OP getRawValue(rValue));                                              \
 	}
 
 DEFINE_BINARY_OPERATOR_HELPER(+, add, ADD, COMMON_RETURN_TYPE)
@@ -268,17 +268,17 @@ DEFINE_BINARY_OPERATOR_HELPER(/, div, DIV, COMMON_RETURN_TYPE)
 
 DEFINE_BINARY_OPERATOR_HELPER(%, mod, MOD, COMMON_RETURN_TYPE)
 
-DEFINE_BINARY_OPERATOR_HELPER(==, eq, EQ, val<bool>)
+DEFINE_BINARY_OPERATOR_HELPER(==, eq, EQ, bool)
 
-DEFINE_BINARY_OPERATOR_HELPER(!=, neq, NEQ, val<bool>)
+DEFINE_BINARY_OPERATOR_HELPER(!=, neq, NEQ, bool)
 
-DEFINE_BINARY_OPERATOR_HELPER(<, lt, LT, val<bool>)
+DEFINE_BINARY_OPERATOR_HELPER(<, lt, LT, bool)
 
-DEFINE_BINARY_OPERATOR_HELPER(<=, lte, LTE, val<bool>)
+DEFINE_BINARY_OPERATOR_HELPER(<=, lte, LTE, bool)
 
-DEFINE_BINARY_OPERATOR_HELPER(>, gt, GT, val<bool>)
+DEFINE_BINARY_OPERATOR_HELPER(>, gt, GT, bool)
 
-DEFINE_BINARY_OPERATOR_HELPER(>=, gte, GTE, val<bool>)
+DEFINE_BINARY_OPERATOR_HELPER(>=, gte, GTE, bool)
 
 DEFINE_BINARY_OPERATOR_HELPER(>>, shr, RSH, DEDUCT_RETURN_TYPE(>>))
 

diff --git a/nautilus/test/common/ExpressionFunctions.hpp b/nautilus/test/common/ExpressionFunctions.hpp
@@ -63,7 +63,15 @@ val<double> doubleAddExpression(val<double> x) {
 	return x + y;
 }
 
+auto subInt8AndInt8(val<int8_t> x, val<int8_t> y) {
+	const auto result = x - y;
+	return result;
+}
 
+auto addInt8AndInt32(val<int8_t> x, val<int32_t> y) {
+	const auto result = x + y;
+	return result;
+}
 
 val<double> castFloatToDoubleAddExpression(val<float> x) {
 	val<double> y = 7.0;

diff --git a/nautilus/test/data/expression-tests/after_ssa/addInt8AndInt32.trace b/nautilus/test/data/expression-tests/after_ssa/addInt8AndInt32.trace
@@ -0,0 +1,4 @@
+B0($1:i8,$2:i32)
+	CAST	$3	$1	:i32
+	ADD	$4	$3	$2	:i32
+	RETURN	$4	:i32
diff --git a/nautilus/test/data/expression-tests/after_ssa/subInt8AndInt8.trace b/nautilus/test/data/expression-tests/after_ssa/subInt8AndInt8.trace
@@ -0,0 +1,3 @@
+B0($1:i8, $2:i8):
+	SUB     $3:i8, $1:i8, $2:i8
+	RETURN	$3	:i8
diff --git a/nautilus/test/data/expression-tests/ir/addInt8AndInt32.trace b/nautilus/test/data/expression-tests/ir/addInt8AndInt32.trace
@@ -0,0 +1,8 @@
+NautilusIr {
+execute() {
+Block_0($1:i8, $2:i32):
+	$3 = $1 cast_to i32 :i32
+	$4 = $3 + $2 :i32
+	return ($4) :i32
+}
+} //NESIR
diff --git a/nautilus/test/data/expression-tests/ir/subInt8AndInt8.trace b/nautilus/test/data/expression-tests/ir/subInt8AndInt8.trace
@@ -0,0 +1,7 @@
+NautilusIr {
+execute() {
+Block_0($1:i8, $2:i8):
+	$3 = $1 - $2 :i8
+	return ($3) :i8
+}
+} //NESIR
diff --git a/nautilus/test/data/expression-tests/tracing/addInt8AndInt32.trace b/nautilus/test/data/expression-tests/tracing/addInt8AndInt32.trace
@@ -0,0 +1,4 @@
+B0($1:i8,$2:i32)
+	CAST	$3	$1	:i32
+	ADD	$4	$3	$2	:i32
+	RETURN	$4	:i32
diff --git a/nautilus/test/data/expression-tests/tracing/subInt8AndInt8.trace b/nautilus/test/data/expression-tests/tracing/subInt8AndInt8.trace
@@ -0,0 +1,3 @@
+B0($1:i8,$2:i8)
+	SUB	$3	$1	$2	:i8
+	RETURN	$3	:i8
diff --git a/nautilus/test/execution-tests/CMakeLists.txt b/nautilus/test/execution-tests/CMakeLists.txt
@@ -14,7 +14,7 @@ list(APPEND CMAKE_MODULE_PATH ${catch2_SOURCE_DIR}/extras)
 
 catch_discover_tests(nautilus-execution-tests EXTRA_ARGS --allow-running-no-tests)
 
-if (ENABLE_TRACING AND (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"))
+if (ENABLE_TRACING)
     # using Clang
     add_executable(nautilus-tracing-tests
             TracingTest.cpp

diff --git a/nautilus/test/execution-tests/ExecutionTest.cpp b/nautilus/test/execution-tests/ExecutionTest.cpp
@@ -251,6 +251,22 @@ void expressionTests(engine::NautilusEngine& engine) {
 		REQUIRE(f((double) -7) == 0);
 		REQUIRE(f((double) -14) == -7);
 	}
+
+	SECTION("subInt8AndInt8") {
+		auto f = engine.registerFunction(subInt8AndInt8);
+		REQUIRE(f((int8_t) 1, (int8_t) 2) == -1);
+		REQUIRE(f((int8_t) 123, (int8_t) 123) == 0);
+		REQUIRE(f((int8_t) 78, (int8_t) 70) == 8);
+		REQUIRE(f((int8_t) -128, (int8_t) 1) == 127);
+	}
+
+	SECTION("addInt8AndInt32") {
+		auto f = engine.registerFunction(addInt8AndInt32);
+		REQUIRE(f((int8_t) 1, (int32_t) 2) == 3);
+		REQUIRE(f((int8_t) 123, (int32_t) 123) == 246);
+		REQUIRE(f((int8_t) 78, (int32_t) 70) == 148);
+		REQUIRE(f((int8_t) -128, (int32_t) -1) == -129);
+	}
 }
 
 void controlFlowTest(engine::NautilusEngine& engine) {

diff --git a/nautilus/test/execution-tests/TracingTest.cpp b/nautilus/test/execution-tests/TracingTest.cpp
@@ -188,6 +188,8 @@ TEST_CASE("Expression Trace Test") {
 	    {"shiftRight_i8", details::createFunctionWrapper(shiftRight<int8_t>)},
 	    {"negate_i8", details::createFunctionWrapper(negate<int8_t>)},
 	    {"logicalNot_bool", details::createFunctionWrapper(logicalNot<bool>)},
+	    {"subInt8AndInt8", details::createFunctionWrapper(subInt8AndInt8)},
+	    {"addInt8AndInt32", details::createFunctionWrapper(addInt8AndInt32)},
 	};
 	runTraceTests("expression-tests", tests);
 }