Added support for arithmetic with different types by dghosef · Pull Request #1 · sillycross/PochiVM

dghosef · 2021-07-13T07:47:30Z

Change Pochi addition semantics to implicitly convert integers/floats/doubles when adding different types. Doesn't work for 8 bit and 16 bit values.

I'm not sure if I should refactor the tests so that they are easier to reuse if we were to ever add implicit conversion to other operations or if we're planning on doing that in the near future.

sillycross

I added comments to your code. Regarding tests: I don't really care about the code quality of tests, as long as it is not overly mystic (and requires a lot of time to understand when the test fails)..

sillycross · 2021-07-13T08:18:40Z

+          typename = std::enable_if_t<AstTypeHelper::primitive_type_supports_binary_op<T, AstTypeHelper::BinaryOps::ADD>::value> >
+Value<typename std::common_type<T, U>::type> operator+(const Value<T> &lhs, const Value<U> &rhs)
+{
+    using return_type = typename std::common_type<T, U>::type;


In general, when you work on a codebase, your code should follow the existing naming convention and code style.
I'm not saying that my naming convention/code style is good or that you should use it in your own project (actually, I'm using Qt style, which is not really a dominant style), but you should still follow it to be consistent with the rest of the code. So please use 'ReturnType' instead.

sillycross · 2021-07-13T08:23:33Z

+    static_assert(std::is_signed<T>::value == std::is_signed<U>::value ||
+                      std::is_floating_point<return_type>::value,
+                  "cannot add two values of different signedness");
+    if(!std::is_same<T, return_type>::value)


The easiest way to avoid bugs is defensive programming: asserts, asserts, more asserts! The goal is that your program never fails elsewhere: if it passes all asserts, it should work correctly; and if it fails somewhere, it should always fail in an assertion, instead of running all the way down and crashes somewhere or produces a wrong result (because in that case you would have to debug where exactly it went wrong).
So please make all your assumptions explicit using asserts, even if they might be semi-obvious to you, or if it has been asserted in the callee (asserting twice doesn't hurt you, and caller-callee assumptions can change at any time).
Specifically, here, the assert you want to add is "rhs has the same type as ReturnType". So you should write
if constexpr(....) {
static_assert(.....);
return ....;
}

sillycross · 2021-07-13T08:24:33Z

+                      std::is_floating_point<return_type>::value,
+                  "cannot add two values of different signedness");
+    if(!std::is_same<T, return_type>::value)
+        return Value<return_type>(new AstArithmeticExpr(AstArithmeticExprType::ADD,


Always write if (...) { ... }
Do not omit curly bracket. Omitting curly brackets is source of bugs.

sillycross · 2021-07-13T08:32:14Z

+    #pragma clang diagnostic ignored "-Wimplicit-int-float-conversion"
+    #pragma clang diagnostic ignored "-Wdouble-promotion"
+
+    auto expected = lhs + rhs;


I'm a bit suspicious if this would work correctly. If I recall it correctly C++ has the weird thing that promotes everything <32bits to 32 bits (which Pochi currently does not support right now: if you add two uint8_t together you still get a uint8_t, not a uint32_t). So this will likely fail in overflow cases if you add, say, a uint8_t with a uint16_t (e.g 1 + 65535), but I'm not sure.

sillycross · 2021-07-14T06:48:13Z

+        return Value<ReturnType>(new AstArithmeticExpr(AstArithmeticExprType::ADD,
+                                                        lhs.__pochivm_value_ptr, StaticCast<ReturnType>(rhs).__pochivm_value_ptr));
+    }
+    return Value<ReturnType>(new AstArithmeticExpr(AstArithmeticExprType::ADD,


For this branch, you should assert that T=U=ReturnType

sillycross · 2021-07-14T06:49:27Z

+                  "cannot add two values of different signedness");
+    if constexpr (!std::is_same<T, ReturnType>::value)
+    {
+        static_assert(std::is_same<ReturnType, U>::value, "rhs type is not the same as return type");


nit: this assertion is different from the above assertion: if this assertion fails, it indicates a bug in our code (not user code). I would recommend add "internal bug: " to the start of the message. Same below.

sillycross · 2021-07-14T19:38:56Z

+// 
+template <typename T, typename U>
+struct ArithReturnType {
+    typedef typename std::conditional<sizeof(T) <= sizeof(int16_t) || sizeof(U) <= sizeof(int16_t),


Use C++ 'using' instead of C 'typedef': 'using' is a strictly better replacement for 'typedef' in terms of readability.

sillycross · 2021-07-14T19:41:32Z

+                                                        lhs.__pochivm_value_ptr, StaticCast<ReturnType>(rhs).__pochivm_value_ptr));
+    }
+    else {
+        static_assert(std::is_same<T, U>::value, "internal bug: lhs and rhs don't have the same time");


I know that the two if branches above is already guaranteeing that T=U=ReturnType, but I would still make the assertion be 'std::is_same<T, U>::value && std::is_same<T, ReturnType>::value'. It makes the correctness of your code obvious: the best code is the code that is obviously correct.

sillycross · 2021-07-14T19:43:37Z

+    #undef F
+}
+
+void TestAdditionUnsignedWithPromotion() {


Can you add a few lines of comments explaining what you are testing?

dghosef · 2021-07-21T09:25:56Z

I rewrote the tests to include testing of addition with literals. Currently the tests are much less comprehensive than they were and take roughly 3.5 times as long to complete(2000ms vs 7000 ms on my machine because of all the new permutations of types and because of the tests with literals. When I tried to make them more comprehensive/test more values, the runtime becomes way too long. I think the problem is having the literal tests significantly slowed down the process because the function had to be recompiled for every different literal.

If you think it would better, I can just restore the old tests and modify those to go through all the different permutations of types. Now that I'm thinking about it, that might just be the better move because those were more comprehensive...

sillycross · 2021-07-21T20:45:42Z

Thanks for writing all the tests. I haven't reviewed your code yet, but to answer your question: I would recommend to not test LLVM backend (or only test a few of them) for the 'op with literal' case (i.e., keep the 'op with literal' cases, but for those cases only test the interpreter and c&p backend). There are two reasoning behind this:
(1) I didn't check myself but I think most of the time should be spent in LLVM compilation. So skipping the LLVM backend test for 'op with literal' case should be able to bring the time down.
(2) The LLVM backend does not treat 'op with literal' special in any manner, so there isn't a strong reason that something could go wrong only in the 'op with literal' case.

sillycross

Sorry for the delay (I missed your updated diff). I have added comments in the code.

sillycross · 2021-07-27T22:59:27Z

+    {
+        static_assert(std::is_same<ReturnType, U>::value, "internal bug: rhs type is not the same as return type");
+        return Value<ReturnType>(new AstArithmeticExpr(expr_type,
+                                                        StaticCast<ReturnType>(lhs).__pochivm_value_ptr, rhs.__pochivm_value_ptr));


align function parameters please (you have one more space)

sillycross · 2021-07-27T23:01:24Z

@@ -117,111 +117,223 @@ Value<T>::operator Value<U>() const

 // Arithmetic ops convenience operator overloading
 //


move this comment to the right place, and add comment on what your 'DoArith' is doing

sillycross · 2021-07-27T23:03:13Z

-template<typename T, typename = std::enable_if_t<
-             AstTypeHelper::primitive_type_supports_binary_op<T, AstTypeHelper::BinaryOps::ADD>::value> >
-Value<T> operator+(const Value<T>& lhs, const Value<T>& rhs)
+template <AstTypeHelper::BinaryOps Binop, typename T, typename U,


What is the point of this 'Binop' if you are having an 'expr_type' parameter already? This is extremely fragile.

sillycross · 2021-07-27T23:06:50Z

+Value<typename AstTypeHelper::ArithReturnType<T, U>::type> operator+(const Value<T>& lhs, const Value<U>& rhs)
 {
-    return Value<T>(new AstArithmeticExpr(AstArithmeticExprType::ADD, lhs.__pochivm_value_ptr, rhs.__pochivm_value_ptr));
+    return DoArith<AstTypeHelper::BinaryOps::ADD>(lhs, rhs, AstArithmeticExprType::ADD);


As I said above, this is very fragile: you need to specify the 'ADD' twice and they must be kept in sync. And even worse, your 'DoArith' function does not even have an assertion that asserts that they are in sync. (the fix is not to add an assertion though. You should remove the need to pass one of the parameter.)

sillycross · 2021-07-27T23:09:26Z

+    else if constexpr (!std::is_same<U, ReturnType>::value) 
+    {
+        static_assert(std::is_same<ReturnType, T>::value, "internal bug: lhs type is not the same as the computed common type");
+		ReturnType casted_rhs = static_cast<ReturnType>(rhs);


Do not use tab to indent, use space. As you can see, using tabs works on your computer but fails to indent correctly here on github (if you are using an IDE, you can usually find an option to change this behavior).

sillycross · 2021-07-27T23:11:37Z

+
+// Do arithmetic of form runtime value OP literal
+//
+template <AstTypeHelper::BinaryOps Binop, typename T, typename U,


can you reuse the logic of DoArith, so that you write something like return DoArith(lhs, Literal<U>(rhs), expr_type)?
I didn't try myself so let me know if you think this approach could not work.

sillycross · 2021-07-27T23:12:14Z

+
+// Do arithmetic of form literal OP runtime value
+//
+template <AstTypeHelper::BinaryOps Binop, typename T, typename U,


same question as above

sillycross · 2021-07-27T23:13:57Z

 using namespace PochiVM;

+// TODO: Test comparison ops, unsigned math, add back all old tests that I deleted,
+// make signed tests more comprehensive, literal tests, 


address the TODO?

sillycross

Thanks for the update. I have added comments.

sillycross · 2021-07-30T00:59:04Z

 namespace PochiVM
 {

+template<typename T>


why do you need this?

Literal returns a Value so Value has to be forward declared.

that's why I'm saying if you can just move the 'Literal' function up a bit..

sillycross · 2021-07-30T00:59:34Z

 template<typename T>
 class ConstPrimitiveReference;

+template<typename T>


can you just move the 'literal' function upper a bit instead of having a forward definition?

sillycross · 2021-07-30T01:16:48Z

+Value<typename AstTypeHelper::ArithReturnType<T, U>::type> operator+(const Value<T>& lhs, U rhs)
 {
-    return Value<T>(new AstArithmeticExpr(AstArithmeticExprType::ADD, lhs.__pochivm_value_ptr, new AstLiteralExpr(TypeId::Get<T>(), &rhs)));
+    return DoArith<AstArithmeticExprType::ADD>(lhs, Literal<U>(rhs));


one issue with your current approach is that code like 'a+1' where a is int64_t would generate AST like Add(a, StaticCast<uint64_t>(Literal<int32_t>(1)), which is kind of bad. You can fix this by doing the rewrite in DoArith. (i.e. in DoArith, check if the side to be StaticCast'ed is a Literal, and if it is, directly cast its content).

I'm not sure how I would directly cast a literal's content. Do you think I should add a GetAs<T>() method to the AstLiteralExpr class that returns a literal's value in the form of type T? Then I can cast that value to the desired type and put that in a new Literal? Alternatively, I could just go back to the old method with multiple overloads for ltierals on the left/right.

yes that's definitely one way to do it.

sillycross · 2021-07-30T01:20:38Z

+template <AstArithmeticExprType expr_type, typename T, typename U, 
+          typename = std::enable_if_t<AstTypeHelper::primitive_type_supports_arithmetic_expr_type<T, expr_type>::value>,
+          typename = std::enable_if_t<AstTypeHelper::primitive_type_supports_arithmetic_expr_type<U, expr_type>::value> >
+Value<typename AstTypeHelper::ArithReturnType<T, U>::type> DoArith(const Value<T>& lhs, const Value<U>& rhs)


The 'DoArith' name is very blurry (one cannot understand or even guess what this function is doing from its name). Can you change it to a more descriptive name?

sillycross

I have added comments.

sillycross · 2021-07-30T09:37:19Z

+    static_assert(AstTypeHelper::may_static_cast<T, U>::value, "cannot static_cast T to U");
+    if(src.__pochivm_value_ptr->GetAstNodeType() == AstNodeType::AstLiteralExpr)
+    {
+        return Literal<U>(static_cast<U>(reinterpret_cast<AstLiteralExpr *>(src.__pochivm_value_ptr)->GetAs<T>()));


please go to cppreference.com and learn the different types of CPP casts. Specifically, reinterpret_cast is only used to reinterpret a pointer as an integer or vice versa.
The type of cast you want to do here is a downcast, so you should use either static_cast (silently breaks down if the type is wrong) or dynamic_cast (gracefully provides a runtime error, but incurs a large runtime cost). In our system this is addressed by our assert_cast helper, which does dynamic_cast in debug build but static_cast in non-debug build.

as a side note, using reinterpret_cast to do upcast or downcast is not a style issue but a a correctness issue because it breaks down if the cast results in a pointer shift, see example below:

struct A { int x; }; struct B { int y; }; struct C : A, B {}; C* c = new C(); c->x = 1; c->y = 2; B* b1 = static_cast<B*>(c); printf("%d\n", b1->y); // gives you 2 correctly B* b2 = reinterpret_cast<B*>(c); printf("%d\n", b2->y); // gives you 1 instead of 2

Sorry my bad. That was a stupid mistake

sillycross · 2021-07-30T09:40:21Z

+
+// Helper for Arithmetic ops operator overloading. Returns an arithmetic expression of the form
+// lhs OP rhs where OP is the operator specified by `ExprType`
+//


you should note in the comment that it casts the operands according to the C rule. I would also rename the function to something like 'CastOperandAndDoArithmetic'

sillycross · 2021-07-30T09:40:50Z

 template<typename T>
 class ConstPrimitiveReference;

+


dghosef · 2021-07-30T11:23:48Z

I also added checks to make sure we don't compare bools to non-bools(like 3 == true). I would assume that this is desired behavior, but C++ does allow such comparisons, so I can remove those if we wish to remain consistent with C/C++ behavior

sillycross · 2021-07-30T20:07:14Z

-// If the input is rvalue, the conversion to this struct generates a 'AstRvalueToConstPrimitiveRefExpr' operator.
-//
 template<typename T>
-class ConstPrimitiveReference


Can you explain what did you do in this commit? Is it undoing a previous commit you did? Since I cannot see its effect in the final "file changes"

Earlier you highlighted the extra newline under the ConstPrimitiveReference forward declaration and told me to remove it. In the previous commit I misinterpreted it as you telling me to remove the whole ConstPrimitiveReference forward declaration and move the actual class up there. But then I realized you had only highlighted that one newline. So I restored the old forward declaration and just deleted that one newline.

sillycross

lgtm beside the nit comment. Well done!

sillycross · 2021-07-30T20:41:57Z

+    static_assert(AstTypeHelper::may_static_cast<T, U>::value, "cannot static_cast T to U");
+    if(src.__pochivm_value_ptr->GetAstNodeType() == AstNodeType::AstLiteralExpr)
+    {
+        return Literal<U>(static_cast<U>(assert_cast<AstLiteralExpr *>(src.__pochivm_value_ptr)->template GetAs<T>()));


nit: remove the space between in AstLiteralExpr * to be consistent with the existing code style

sillycross · 2021-08-09T21:31:03Z

Can you rename your commit so it reflect the stuffs you did? (you actually supported not only addition but arithmetics in general, and also a few helpers like C++ Literal op Value)

Adds support for operations like int16_t + int64_t or uint8_t + double(the operand with the smaller size is promoted to the operand with the larger size) and tests for this new functionality. Also adds a few helper functions including StaticCastOrConvertLiteral, CastOperandAndDoArithmetic, and CastOperandAndDoComparison. Also adds a GetAs<T> member to the AstLiteralExpression to get the value of a literal as type T

sillycross reviewed Jul 13, 2021

View reviewed changes

sillycross reviewed Jul 14, 2021

View reviewed changes

sillycross reviewed Jul 27, 2021

View reviewed changes

sillycross reviewed Jul 30, 2021

View reviewed changes

dghosef force-pushed the Overload-add branch from 8d53323 to 9771d2f Compare August 9, 2021 04:37

dghosef force-pushed the Overload-add branch from 9771d2f to 9aec85d Compare August 9, 2021 23:23

dghosef force-pushed the Overload-add branch from 9aec85d to c65f3c6 Compare August 9, 2021 23:24

dghosef changed the title ~~Added implicit promotions for addition and corresponding tests~~ Added implicit promotions for arithmetic and comparisons and corresponding tests Aug 10, 2021

dghosef changed the title ~~Added implicit promotions for arithmetic and comparisons and corresponding tests~~ Added support for arithmetic with different types Aug 10, 2021

sillycross force-pushed the master branch 2 times, most recently from 6519c6d to 2e99da4 Compare September 9, 2021 02:54

		@@ -117,111 +117,223 @@ Value<T>::operator Value<U>() const

		// Arithmetic ops convenience operator overloading
		//

Conversation

dghosef commented Jul 13, 2021

Uh oh!

sillycross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dghosef commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sillycross commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sillycross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sillycross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dghosef Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sillycross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sillycross Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

dghosef commented Jul 21, 2021 •

edited

Loading

sillycross commented Jul 21, 2021 •

edited

Loading

dghosef Jul 30, 2021 •

edited

Loading

sillycross Jul 30, 2021 •

edited

Loading