Add support for compilation-hints proposal #2627
Conversation
…tr_freq` and `metadata.code.call_targets` annotations and sections
Lukasdoe
force-pushed
the
add-compile-hints
branch
from
5aa5685 to
d43cd08
Compare
| name(name), | ||
| type(Type::InstructionFrequency) { | ||
| new (&hint.instruction_frequency) | ||
| InstructionFrequency(instruction_frequency); |
There was a problem hiding this comment.
Can you just do hint.instruction_frequency = instruction_frequency here instead of the placement new?
There was a problem hiding this comment.
Since hint.instruction_frequency is not a valid C++ object, we're not allowed to use its assignment copy operator. In practice, using it should work as expected, but it is still undefined behavior nevertheless (afaik). That's why I opted for just writing over it, which should definitely be safe.
| InstructionFrequency instruction_frequency{}; | ||
| std::vector<CallTarget> call_targets; | ||
| Hint() {} | ||
| ~Hint() {} |
There was a problem hiding this comment.
Do we need the explicit ctor and dtor here?
There was a problem hiding this comment.
Good catch! The explicit constructor is not required, but the noop destructor is required, since the default one is automatically deleted. (and we do destruction from outside the union based on the stored type.
There was a problem hiding this comment.
Nvm, while it works on my machine, the CI compilers complain when I remove the constructor^^
| std::vector<CallTarget> call_targets; | ||
| Hint() {} | ||
| ~Hint() {} | ||
| } hint; |
There was a problem hiding this comment.
Since we need to overwrite the constructor, we need to name the union, afaik. But please correct me if there's a way to define a destructor for an anonymous union in C++.
include/wabt/ir.h
Outdated
|
|
||
| std::string_view name; | ||
| std::vector<uint8_t> data; | ||
| Type type; |
There was a problem hiding this comment.
Does type need to be public here?
include/wabt/ir.h
Outdated
| InstructionFrequency, | ||
| CallTargets | ||
| }; | ||
|
|
||
| explicit CodeMetadataExpr(std::string_view name, |
There was a problem hiding this comment.
IIUC we don't normally use/need the explicit keyword except for single argument ctors?
Title says it all, this PR adds support for compilation hints.