Value type improvements #423

chfast · 2020-07-15T16:39:33Z

Depends on #419

gumb0 · 2020-07-15T17:48:37Z

macOS builds failed

chfast · 2020-07-16T11:59:01Z

macOS builds failed

It looks on macOS the uint64_t is unsigned long long so we need more overloads.

codecov · 2020-07-16T12:01:01Z

Codecov Report

Merging #423 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          51       51           
  Lines       14632    14663   +31     
=======================================
+ Hits        14565    14596   +31     
  Misses         67       67

gumb0 · 2020-07-30T15:00:19Z

lib/fizzy/execute.cpp

-    using T = decltype(op(stack.top(), stack.top()));
-    const auto val2 = static_cast<T>(stack.pop());
-    const auto val1 = static_cast<T>(stack.top());
-    stack.top() = static_cast<std::make_unsigned_t<T>>(op(val1, val2));


How come make_unsigned is not needed anymore? I think it was need for signed 32-bit operations, where without it it would incorrectly extend the sign to 64 bit.

Ah I think now constructor Value(int32_t v) does this.

Is it better to have it in the constructor or make it more explicit here? From a reader's perspective explicitness here is nice.

Do we recall the test case while it had to be converted between signed/unsigned in this function?

All "signed integer" instructions relay on this.

I think I like it better when Value itself encapsulates correct conversion int32_t->Value.

make_unsigned_t here looked to me more like a hack to handle an edge case...

We could make the conversions from signed integers explicit. This is manageable in interpreter implementation, but inconvenient in tests and examples. I.e. all invokes with example {0, 1, 2} stops working (as naked integer literal is of type int) and you have to use {0u, 1u, 2u}.

Personally, I would leave it as is for now. And we can review Value type design after having floating point type in. And possibly i32 fields as well.

I'm fine keeping it this way but would prefer a simple comment that the result can be signed or unsigned and its conversion is handled in Value.

Added comments and used Value explicitly (no implicit conversion although available).

Hope it did not change runtimes lol.

gumb0 · 2020-07-30T15:10:25Z

lib/fizzy/value.hpp

@@ -13,8 +13,19 @@ union Value
    uint64_t i64;

    Value() = default;
-    constexpr Value(uint64_t v) noexcept : i64{v} {}
+    constexpr Value(unsigned int v) noexcept : i64{v} {}


Maybe a comment why we need these unsigned constuctors?

Instead of having 5 constructor for ints + deleted constructors, I can do all of that with a template constructor with some enable_if and some other magic. But I'm not sure we want that now. Maybe it's better to wait for floats before more cleanups.

gumb0 · 2020-07-30T16:48:56Z

test/utils/fizzy_engine.cpp

-    const auto status =
-        fizzy::execute(*m_instance, static_cast<uint32_t>(func_ref), {first_arg, args.size()});
+    const auto status = fizzy::execute(
+        *m_instance, static_cast<uint32_t>(func_ref), span<const Value>(first_arg, args.size()));


Is this change required?

Yes. Otherwise compiler thinks these are two values: first_arg -> bool -> Value and args.size() -> size_t -> Value.

I see. Fo first_arg it's probably pointer to integer conversion, rather than bool, I think bool -> Value doesn't work anymore, you deleted the tests for it.

Without the change it resolves overloading to Value(bool) what gives the compilation error (as this constructor is deleted).

axic · 2020-07-30T17:08:39Z

lib/fizzy/execute.cpp

@@ -1014,7 +1014,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, span<const Value>
        case Instr::i32_eqz:
        {
            const auto value = static_cast<uint32_t>(stack.pop());
-            stack.push(value == 0);
+            stack.push(uint32_t{value == 0});


Why do we need to make this clear? I know eqz returns 32-bit but we said we don't care because it is a union.

Does this have any effect on speed?

The Value(bool) is deleted to avoid some surprises. So the bool expression must be converted to uint32_t (or uint64_t, but uint32_t seems nicer).

Does this have any effect on speed?

Nope.

chfast · 2020-07-30T17:21:12Z

Cleaned up the git history.

chfast · 2020-07-30T18:13:45Z

Benchmarks (no change):

fizzy/execute/blake2b/512_bytes_rounds_1_mean                     +0.0011         +0.0011            87            87            87            87
fizzy/execute/blake2b/512_bytes_rounds_16_mean                    +0.0006         +0.0006          1310          1311          1310          1311
fizzy/execute/ecpairing/onepoint_mean                             +0.0039         +0.0039        429238        430925        429239        430929
fizzy/execute/keccak256/512_bytes_rounds_1_mean                   -0.0056         -0.0056           105           104           105           104
fizzy/execute/keccak256/512_bytes_rounds_16_mean                  -0.0089         -0.0089          1530          1517          1530          1517
fizzy/execute/memset/256_bytes_mean                               -0.0033         -0.0033             7             7             7             7
fizzy/execute/memset/60000_bytes_mean                             +0.0004         +0.0004          1610          1610          1610          1610
fizzy/execute/mul256_opt0/input0_mean                             +0.0010         +0.0010            27            27            27            27
fizzy/execute/mul256_opt0/input1_mean                             +0.0015         +0.0015            27            27            27            27
fizzy/execute/sha1/512_bytes_rounds_1_mean                        -0.0011         -0.0011            95            95            95            95
fizzy/execute/sha1/512_bytes_rounds_16_mean                       +0.0004         +0.0004          1323          1324          1323          1324
fizzy/execute/sha256/512_bytes_rounds_1_mean                      +0.0064         +0.0064            99           100            99           100
fizzy/execute/sha256/512_bytes_rounds_16_mean                     +0.0025         +0.0025          1377          1380          1377          1380
fizzy/execute/micro/eli_interpreter/halt_mean                     +0.0018         +0.0018             0             0             0             0
fizzy/execute/micro/eli_interpreter/exec105_mean                  -0.0022         -0.0022             5             5             5             5
fizzy/execute/micro/factorial/10_mean                             +0.0020         +0.0020             0             0             0             0
fizzy/execute/micro/factorial/20_mean                             -0.0211         -0.0211             1             1             1             1
fizzy/execute/micro/fibonacci/24_mean                             -0.0103         -0.0103          7253          7178          7253          7178
fizzy/execute/micro/host_adler32/1_mean                           -0.0016         -0.0016             0             0             0             0
fizzy/execute/micro/host_adler32/100_mean                         +0.0013         +0.0013             3             3             3             3
fizzy/execute/micro/host_adler32/1000_mean                        +0.0086         +0.0086            30            30            30            30
fizzy/execute/micro/spinner/1_mean                                -0.0060         -0.0061             0             0             0             0
fizzy/execute/micro/spinner/1000_mean                             -0.0004         -0.0004            10            10            10            10

axic · 2020-07-30T18:17:38Z

lib/fizzy/execute.cpp

-    const auto val2 = static_cast<T>(stack.pop());
-    const auto val1 = static_cast<T>(stack.top());
-    stack.top() = static_cast<std::make_unsigned_t<T>>(op(val1, val2));
+    using T = decltype(op({}, {}));


Hm, what does this do with {}, {}?

The {} in f(T) more/less mean "construct the required type T with default constructor".

E.g. The void func(void*, Options, vector<int>, int) can be invoked as func(nullptr, {}, {}, 0) or even func({}, {}, {}, {}).

Yeah I get that, but in the case of op, which is std::add, std::sub, etc.

Well, op is an instance of e.g. std::add<uint64_t>{} which has operator() overloaded so op(...) expression is valid. The decltype gets the type of this expression.

Okay so this line change is not related to the current PR. Could have been like this prior.

Yes. The {} could have been use instead of stack.top() before. But now this simplifications is required as stack.top() has wrong type in general.

chfast requested review from axic and gumb0 July 15, 2020 16:39

chfast force-pushed the value_type branch from ad86e01 to 808afec Compare July 16, 2020 12:07

chfast force-pushed the value_type_cleanup branch from 246fdf8 to dc41d64 Compare July 16, 2020 12:10

chfast force-pushed the value_type branch from 808afec to 5b064ac Compare July 28, 2020 14:42

chfast force-pushed the value_type branch 3 times, most recently from df0ceb4 to 399b2c3 Compare July 30, 2020 07:35

chfast force-pushed the value_type_cleanup branch from dc41d64 to 068f218 Compare July 30, 2020 14:52

gumb0 reviewed Jul 30, 2020

View reviewed changes

Base automatically changed from value_type to master July 30, 2020 15:23

chfast force-pushed the value_type_cleanup branch 3 times, most recently from 54cd311 to 21a8bbe Compare July 30, 2020 16:38

chfast requested a review from gumb0 July 30, 2020 16:38

gumb0 reviewed Jul 30, 2020

View reviewed changes

gumb0 approved these changes Jul 30, 2020

View reviewed changes

axic reviewed Jul 30, 2020

View reviewed changes

chfast added 2 commits July 30, 2020 19:19

Add Value constructor overloads for integer types

249da86

Add Value::as<T>() helper

e4ba222

chfast force-pushed the value_type_cleanup branch from 21a8bbe to d90f329 Compare July 30, 2020 17:19

chfast mentioned this pull request Jul 30, 2020

Implement floating point const and add #424

Merged

axic reviewed Jul 30, 2020

View reviewed changes

Simplify {unary,binary}_op() implementation

accd4b2

chfast force-pushed the value_type_cleanup branch from d90f329 to accd4b2 Compare July 30, 2020 19:25

axic approved these changes Jul 30, 2020

View reviewed changes

chfast merged commit 40c459e into master Jul 30, 2020

chfast deleted the value_type_cleanup branch July 30, 2020 20:05

axic mentioned this pull request Jul 31, 2020

Replace static_casts with Value::as<> #448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Value type improvements #423

Value type improvements #423

chfast commented Jul 15, 2020 •

edited by gumb0

Loading

gumb0 commented Jul 15, 2020

chfast commented Jul 16, 2020

codecov bot commented Jul 16, 2020 •

edited

Loading

gumb0 Jul 30, 2020

gumb0 Jul 30, 2020

axic Jul 30, 2020

chfast Jul 30, 2020

gumb0 Jul 30, 2020

chfast Jul 30, 2020

axic Jul 30, 2020

chfast Jul 30, 2020

axic Jul 30, 2020

gumb0 Jul 30, 2020 •

edited

Loading

chfast Jul 30, 2020

chfast Jul 30, 2020

gumb0 Jul 30, 2020

chfast Jul 30, 2020

gumb0 Jul 30, 2020

chfast Jul 30, 2020

axic Jul 30, 2020

axic Jul 30, 2020

chfast Jul 30, 2020

chfast Jul 30, 2020

chfast commented Jul 30, 2020

chfast commented Jul 30, 2020

axic Jul 30, 2020

chfast Jul 30, 2020

axic Jul 30, 2020

chfast Jul 30, 2020

axic Jul 30, 2020

chfast Jul 30, 2020

Value type improvements #423

Value type improvements #423

Conversation

chfast commented Jul 15, 2020 • edited by gumb0 Loading

gumb0 commented Jul 15, 2020

chfast commented Jul 16, 2020

codecov bot commented Jul 16, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jul 30, 2020

chfast commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jul 15, 2020 •

edited by gumb0

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

gumb0 Jul 30, 2020 •

edited

Loading