[WIP] 32 bit support #317
[WIP] 32 bit support #317
Conversation
There was a problem hiding this comment.
A good start to integrating the rv32 ISA. Most of the riscv namespace changes look good, I'm unable to comment on the validity of the logic/functionality due to my lacking knowledge of the ISA. There are some changes which I feel could be combined (e.g. the logic for ELF32/ELF64) to reduce code duplication and improve readability.
I don't think we'd be happy with upstreaming the tracing logic in its current format. We've had some internal discussions regarding its implementation but haven't had time to settle on a decision on how to do it. Also, the use of 32/64 bit architecture variables in the parent architecture class doesn't match our current idea of an agnostic arch class. Will have to think about moving this notion to be isolated to the riscv arch class or re-evaluating the content of the parent arch class.
The changes to the emulation core seem good (Other than the RISCV-specific logic which I'm aware will be removed) and the new pipeline buffer class looks promising.
Some of the changes to the RegisterValue.hh header are confusing, we'll need a better explanation for the changes made before moving forward with any decision.
Finally, there's a general need for code clean-up/proper formatting applied throughout the project.
| @@ -14,6 +14,12 @@ using MacroOp = std::vector<std::shared_ptr<Instruction>>; | |||
|
|
|||
| namespace arch { | |||
|
|
|||
| /** Modes. Assume only has 32-bit and 64-bit. */ | |||
| enum arch_mode { | |||
There was a problem hiding this comment.
Please use the enum class to establish an explicit type, and parent it on uint8_t to reduce size usage.
Also please capitalise arch_mode.
You can static_cast the enum values to be used as an index or anything else.
src/include/simeng/RegisterValue.hh
Outdated
| @@ -26,10 +26,16 @@ class RegisterValue { | |||
| * number of bytes (defaulting to the size of the template type). */ | |||
| template <class T, | |||
| typename std::enable_if_t<!std::is_pointer_v<T>, T>* = nullptr> | |||
| RegisterValue(T value, uint16_t bytes = sizeof(T)) : bytes(bytes) { | |||
| RegisterValue(T value, uint16_t bytes = sizeof(T), bool relaxFor32 = true) : bytes(bytes) { | |||
There was a problem hiding this comment.
I think the below statement should be added to this constructor to avoid the case when the bytes argument provided is less than sizeof(T)
bytes = std::max(bytes, sizeof(T));
I think having bytes being less than sizeof(T) doesn't make sense and is dangerous; As it will lead to data truncation. Which I'm not sure why would be needed if a smaller data type can be used. Even if something like this is intentional I still think it should not be allowed.
There was a problem hiding this comment.
A similar statement should be added in the constructor RegisterValue(const char* ptr, uint16_t bytes, uint16_t capacity)
src/include/simeng/RegisterValue.hh
Outdated
| " to access a RegisterValue as a datatype larger than the " | ||
| "data held" ); | ||
| if(!relaxedFor32bit_) { // maybe #ifdef if it makes slower? | ||
| assert(sizeof(T) <= bytes && |
src/include/simeng/RegisterValue.hh
Outdated
| assert(sizeof(T) <= bytes && | ||
| "Attempted to access a RegisterValue as a datatype larger than the " | ||
| "data held"); | ||
| assert((sizeof(T) <= bytes || (bytes == 4 && sizeof(T) == 8)) && "Attempted" |
There was a problem hiding this comment.
Why has the assertion been changed? it has been changed to allow exactly what the first part of the expression is trying to avoid i.e. not allowing a value of smaller size held inside RegisterValue to be extracted as a larger data type. I'm not a fan and don't think this should be allowed, as the value extract will be truncated by the reinterpret_cast.
| @@ -129,6 +140,9 @@ class RegisterValue { | |||
| /** The underlying local member value. Aligned to 8 bytes to prevent | |||
| * potential alignment issue when casting. */ | |||
| alignas(8) char value[MAX_LOCAL_BYTES]; | |||
|
|
|||
| /** Switch for different assert checking */ | |||
| bool relaxedFor32bit_; | |||
src/include/simeng/RegisterValue.hh
Outdated
| if (isLocal()) { | ||
| T* view = reinterpret_cast<T*>(this->value); | ||
| view[0] = value; | ||
| if (sizeof(T) > bytes) { // e.g. when T is int64 and bytes is 4 |
There was a problem hiding this comment.
What would be the use case for such an addition? This will cause a truncation during reinterpret_cast. This might work for a specific use case but could cause problems in the future if something like this is allowed.
Why can't you use int32 instead?
This is a temporary PR to review initial 32 bit support provided by Huawei. Information regarding the changes made is available in README_RV32.md