Replace static_casts with Value::as<> #448

axic · 2020-07-31T18:16:07Z

This should have been done in #423.

Depends on #446.

axic · 2020-07-31T18:24:25Z

lib/fizzy/execute.cpp

@@ -687,7 +688,8 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, span<const Value>
            const auto br_table_size = read<uint32_t>(immediates);
            const auto arity = read<uint32_t>(immediates);

-            const auto br_table_idx = stack.pop();
+            // TODO: is uint64_t the correct stack type here?
+            const auto br_table_idx = stack.pop().as<uint64_t>();


@gumb0 do you know this top of your head: what is the idx type for br_table?

We have a nice table to look it up :)

fizzy/lib/fizzy/instructions.cpp

Line 29 in 0728606

/* br_table = 0x0e */ {{ValType::i32}, {}},

Here's the validation spec that defines input value as i32
https://webassembly.github.io/spec/core/valid/instructions.html#xref-syntax-instructions-syntax-instr-control-mathsf-br-table-l-ast-l-n

axic · 2020-07-31T18:24:47Z

lib/fizzy/execute.cpp

@@ -716,7 +718,8 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, span<const Value>
            const auto expected_type_idx = read<uint32_t>(immediates);
            assert(expected_type_idx < instance.module.typesec.size());

-            const auto elem_idx = stack.pop();
+            // TODO: is this the correct stack type?
+            const auto elem_idx = stack.pop().as<uint64_t>();


@gumb0 this here is elem_idx type for call_indirect

This one is also i32 https://webassembly.github.io/spec/core/valid/instructions.html#xref-syntax-instructions-syntax-instr-control-mathsf-call-indirect-x

codecov · 2020-07-31T18:28:53Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #448   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files          52       52           
  Lines       14937    14939    +2     
=======================================
+ Hits        14863    14865    +2     
  Misses         74       74

lib/fizzy/execute.cpp

chfast · 2020-07-31T19:33:38Z

lib/fizzy/execute.cpp

@@ -344,8 +344,9 @@ template <typename DstT>
 inline bool store_into_memory(
    bytes& memory, OperandStack& stack, const uint8_t*& immediates) noexcept
 {
-    const auto value = static_cast<DstT>(stack.pop());
-    const auto address = static_cast<uint32_t>(stack.pop());
+    // TODO: consider adding uint8_t/uint16_t overloads to as()


Not good idea.

lib/fizzy/execute.cpp

chfast · 2020-07-31T19:34:55Z

lib/fizzy/execute.cpp

@@ -687,7 +688,8 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, span<const Value>
            const auto br_table_size = read<uint32_t>(immediates);
            const auto arity = read<uint32_t>(immediates);

-            const auto br_table_idx = stack.pop();
+            // TODO: is uint64_t the correct stack type here?
+            const auto br_table_idx = stack.pop().as<uint64_t>();


Suggested change

const auto br_table_idx = stack.pop().as<uint64_t>();

const auto br_table_idx = stack.pop().i64;

Here and in other places.

lib/fizzy/execute.cpp

axic · 2020-08-07T10:51:18Z

@chfast how about this now?

axic · 2020-08-07T10:54:41Z

lib/fizzy/execute.cpp

-            const auto value = static_cast<int32_t>(stack.pop());
-            stack.push(static_cast<uint64_t>(int64_t{value}));
+            const auto value = stack.pop().as<int32_t>();
+            stack.push(int64_t{value});


Do we actually need this int64_t{} here? We have an appropriate constructor for Value.

Yes, it is needed as this is the core of this instruction implementation (otherwise for int32_t type the value will be cap to 32 bits). But it may be better to move it to the live above: int64_t{stack.pop().as<int32_t>()}.

Could also squash it into a single one: stack.push(int64_t{stack.pop().as<int32_t>()}.

It all is a matter of preference, perhaps the current one is well balanced for readability.

Fine to leave it too.

chfast · 2020-08-07T11:07:24Z

lib/fizzy/execute.cpp

-            const auto value = static_cast<int32_t>(stack.pop());
-            stack.push(static_cast<uint64_t>(int64_t{value}));
+            const auto value = stack.pop().as<int32_t>();
+            stack.push(int64_t{value});


Yes, it is needed as this is the core of this instruction implementation (otherwise for int32_t type the value will be cap to 32 bits). But it may be better to move it to the live above: int64_t{stack.pop().as<int32_t>()}.

chfast · 2020-08-07T11:08:04Z

lib/fizzy/execute.cpp

@@ -691,7 +693,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, span<const Value>
            const auto br_table_size = read<uint32_t>(immediates);
            const auto arity = read<uint32_t>(immediates);

-            const auto br_table_idx = stack.pop();
+            const auto br_table_idx = stack.pop().as<uint32_t>();


Make sure these functional changes are in a separate commit.

Oh, the _idx stuff is a separate commit, also available as #461.

axic · 2020-08-07T11:51:59Z

Squashed the commits. The first commit can be merged as part of this PR or separately under #461.

@chfast @gumb0 please give a final review

Or use `.i64` directly where appropriate.

axic force-pushed the value-cast branch from d4973d4 to 72549cc Compare July 31, 2020 18:18

axic commented Jul 31, 2020

View reviewed changes

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved

axic requested review from chfast and gumb0 and removed request for chfast July 31, 2020 18:31

axic mentioned this pull request Jul 31, 2020

Implement f32/f64 comparison operators #449

Merged

chfast reviewed Jul 31, 2020

View reviewed changes

gumb0 reviewed Aug 3, 2020

View reviewed changes

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved

axic changed the base branch from master to float-globals August 6, 2020 23:24

axic mentioned this pull request Aug 6, 2020

Properly use i32 for br_table_idx and elem_idx in execute #461

Closed

axic force-pushed the value-cast branch 3 times, most recently from f78903a to 3914eb0 Compare August 6, 2020 23:46

gumb0 force-pushed the float-globals branch 2 times, most recently from f1a036d to 12ac9ef Compare August 7, 2020 09:56

Base automatically changed from float-globals to master August 7, 2020 10:12

axic force-pushed the value-cast branch from 3914eb0 to 8e1197c Compare August 7, 2020 10:51

axic requested review from gumb0 and chfast August 7, 2020 10:51

axic force-pushed the value-cast branch from 8e1197c to 75e094c Compare August 7, 2020 10:54

axic commented Aug 7, 2020

View reviewed changes

chfast approved these changes Aug 7, 2020

View reviewed changes

axic force-pushed the value-cast branch from 75e094c to 59e0ec6 Compare August 7, 2020 11:51

axic force-pushed the value-cast branch from 59e0ec6 to 06c595d Compare August 7, 2020 11:51

axic requested a review from chfast August 7, 2020 11:51

axic force-pushed the value-cast branch from 06c595d to 62234d4 Compare August 7, 2020 11:59

chfast approved these changes Aug 7, 2020

View reviewed changes

axic added 2 commits August 7, 2020 13:21

Properly use i32 for br_table_idx and elem_idx in execute

11be631

Replace static_casts with Value::as<>

9645d57

Or use `.i64` directly where appropriate.

axic force-pushed the value-cast branch from 62234d4 to 9645d57 Compare August 7, 2020 12:21

gumb0 approved these changes Aug 7, 2020

View reviewed changes

axic merged commit c04cfeb into master Aug 7, 2020

axic deleted the value-cast branch August 7, 2020 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace static_casts with Value::as<> #448

Replace static_casts with Value::as<> #448

axic commented Jul 31, 2020 •

edited

Loading

axic Jul 31, 2020

gumb0 Aug 3, 2020

gumb0 Aug 3, 2020

axic Jul 31, 2020

gumb0 Aug 3, 2020

codecov bot commented Jul 31, 2020 •

edited

Loading

chfast Jul 31, 2020

chfast Jul 31, 2020

axic commented Aug 7, 2020

axic Aug 7, 2020 •

edited

Loading

chfast Aug 7, 2020

axic Aug 7, 2020

chfast Aug 7, 2020

chfast Aug 7, 2020

chfast Aug 7, 2020

axic Aug 7, 2020

axic commented Aug 7, 2020

	const auto br_table_idx = stack.pop().as<uint64_t>();
	const auto br_table_idx = stack.pop().i64;

Replace static_casts with Value::as<> #448

Replace static_casts with Value::as<> #448

Conversation

axic commented Jul 31, 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

codecov bot commented Jul 31, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Aug 7, 2020

axic Aug 7, 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

axic commented Aug 7, 2020

axic commented Jul 31, 2020 •

edited

Loading

codecov bot commented Jul 31, 2020 •

edited

Loading

axic Aug 7, 2020 •

edited

Loading