Introduce Value type for operands #419
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chfast
force-pushed
the
value_type
branch
2 times, most recently
from
791cbee
to
f3ae080
Compare
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
==========================================
- Coverage 99.55% 99.54% -0.02%
==========================================
Files 49 51 +2
Lines 14604 14632 +28
==========================================
+ Hits 14539 14565 +26
- Misses 65 67 +2 |
chfast
force-pushed
the
value_type
branch
2 times, most recently
from
e0c084e
to
5ae0db3
Compare
gumb0
reviewed
Jul 15, 2020
| @@ -611,7 +611,7 @@ execution_result execute(Instance& instance, FuncIdx func_idx, span<const uint64 | |||
| const auto& code = instance.module.codesec[code_idx]; | |||
| auto* const memory = instance.memory.get(); | |||
|
|
|||
| std::vector<uint64_t> locals(args.size() + code.local_count, 0); | |||
| std::vector<Value> locals(args.size() + code.local_count); | |||
There was a problem hiding this comment.
Doesn't this leave them uninitialized, because Value default constructor doesn't initialize it?
There was a problem hiding this comment.
No.
- Vector is using value initialization and then copy to init elements.
- For unions, value initialization is zero initialization, i.e.
-
If T is a union type, the first non-static named data member is zero-initialized and all padding is initialized to zero bits.
There was a problem hiding this comment.
It looks the spec for it has changed in C++11, but the result is the same. But having/not-having default constructor that initializes with zero affects GCC code generation (looks like a bug rather) and the version without zeroing default constructor is faster as it fills locals by a loop instead of a memset call.
gumb0
reviewed
Jul 15, 2020
Merged
axic
reviewed
Jul 28, 2020
|
I'm not sure what is going on here. Can someone recheck? |
chfast
force-pushed
the
value_type
branch
2 times, most recently
from
fcdb355
to
df0ceb4
Compare
|
I'm getting different looking results |
gumb0
reviewed
Jul 29, 2020
gumb0
approved these changes
Jul 30, 2020
axic
reviewed
Jul 30, 2020
axic
reviewed
Jul 30, 2020
axic
reviewed
Jul 30, 2020
gumb0
reviewed
Jul 30, 2020
gumb0
approved these changes
Jul 30, 2020
axic
approved these changes
Jul 30, 2020
gumb0
approved these changes
Jul 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Valueis an union for now containing onlyi64value. Later to be extended withf32,f64and maybei32.I added implicit conversions from/to
uint64_tto limit the number of required changes.The conversion from
std::initializer_list<uint64_t>not possible in a type safe manner. I guess we will need something liketemplate<typename T...> TypedArgsto construct a container ofValues. This will also handle usage of different value types in execution invocation.All will get more complicated if we want constructor from other types (e.g.
float,unsigned int) as we will get ambiguities (e.g.0is of typeintand will not know where to go).