adding f and d extesion #36
Conversation
core/AtlasInst.hpp
Outdated
| @@ -54,7 +54,12 @@ namespace atlas | |||
|
|
|||
| uint32_t getOpcodeSize() const { return opcode_size_; } | |||
|
|
|||
| sparta::Register* getRs1() | |||
| uint64_t getRM() | |||
There was a problem hiding this comment.
Method can be made const
core/AtlasInst.hpp
Outdated
| return opcode_info_->getSpecialField(mavis::OpcodeInfo::SpecialField::RM); | ||
| } | ||
|
|
||
| sparta::Register*& getRs1() |
There was a problem hiding this comment.
Curious is there's any advantage in returning a reference pointer...
| @@ -31,7 +31,7 @@ namespace atlas | |||
| static_assert(sizeof(RV) >= sizeof(SIZE)); | |||
|
|
|||
| using Op = | |||
| typename std::conditional<std::is_same_v<RV, RV64>, | |||
| typename std::conditional<std::is_same<RV, RV64>::value, | |||
There was a problem hiding this comment.
I think value is more compliant with the rest of the code with type. Could use is_same_v as well.
There was a problem hiding this comment.
Ah. I suggest fixing the other code. ;)
core/inst_handlers/d/RvdInsts.cpp
Outdated
| const uint64_t rs2_val = inst->getRs2()->dmiRead<uint64_t>(); | ||
| const uint64_t rs3_val = inst->getRs3()->dmiRead<uint64_t>(); | ||
| const uint64_t product = f64_mul(float64_t{rs1_val}, float64_t{rs2_val}).v; | ||
| inst->getRd()->write(f64_add(float64_t{product ^ 1UL << 63}, float64_t{rs3_val}).v); |
There was a problem hiding this comment.
Suggest () around the binary operations.
There was a problem hiding this comment.
Curious... softfloat doesn't have an madd operations? The way this is implemented might lose some precision.
There was a problem hiding this comment.
no it doesn't provide madd. I think there is no surprise it's going to lose precison anyway when the product is big enough.
core/inst_handlers/d/RvdInsts.cpp
Outdated
| const uint64_t rs2_val = inst->getRs2()->dmiRead<uint64_t>(); | ||
| const uint64_t rs3_val = inst->getRs3()->dmiRead<uint64_t>(); | ||
| const uint64_t product = f64_mul(float64_t{rs1_val}, float64_t{rs2_val}).v; | ||
| inst->getRd()->write(f64_sub(float64_t{product ^ 1UL << 63}, float64_t{rs3_val}).v); |
There was a problem hiding this comment.
Suggest () around the binary operations.
| const T imm = inst->hasImmediate() ? inst->getSignExtendedImmediate<T, IMM_SIZE>() : 0; | ||
| const T vaddr = rs1_val + imm; | ||
| state->getTranslationState()->makeTranslationRequest(vaddr, sizeof(T)); | ||
| return nullptr; |
There was a problem hiding this comment.
Is this function a simple placeholder? If it always returns nullptr, then why return anything?
There was a problem hiding this comment.
@kathlenemagnus might have better answer for this. But the work done here is makeTranslationRequest, then at somewhere later getTranslationResult would return the translated PA.
| using S = uint32_t; | ||
| using D = uint64_t; |
There was a problem hiding this comment.
This makes me nervous to see in a header file and in the global atlas namespace. Are these types just for convenience?
There was a problem hiding this comment.
it's for clarity. S = single precision. D = double precision. so when using the template, it won't be confused with RV64 vs RV32, which are again, uint32_t and uint64_t.
theoretically, every extension could have its own namespace.
| static_assert(std::is_same<RV, RV64>::value || std::is_same<RV, RV32>::value); | ||
| static_assert(std::is_same<SIZE, S>::value || std::is_same<SIZE, D>::value); |
| using D = uint64_t; | ||
|
|
||
| template <typename RV, typename SIZE> | ||
| ActionGroup* float_ls_handler(atlas::AtlasState* state, bool load) |
There was a problem hiding this comment.
Can the parameter load be a template parameter?
| const AtlasInstPtr & inst = state->getCurrentInst(); | ||
| const RV paddr = state->getTranslationState()->getTranslationResult().getPaddr(); | ||
|
|
||
| if (load) |
There was a problem hiding this comment.
If load is a template parameters, this can be if constexpr
core/inst_handlers/f/RvfInsts.cpp
Outdated
| /////////////////////////////////////////////////////////////////////// | ||
| const AtlasInstPtr & inst = state->getCurrentInst(); | ||
| const uint64_t rs1_val = inst->getRs1()->dmiRead<uint64_t>(); | ||
| inst->getRd()->write(rs1_val & 0x0000FFFF); |
There was a problem hiding this comment.
Can we define this mask in AtlasTypes.hpp? I'd prefer to avoid using any hard-coded values.
| // END OF SPIKE CODE | ||
| /////////////////////////////////////////////////////////////////////// | ||
| const AtlasInstPtr & inst = state->getCurrentInst(); | ||
| const uint32_t rs1_val = inst->getRs1()->dmiRead<uint64_t>(); |
There was a problem hiding this comment.
We should use the register macros defined here: https://github.com/sparcians/atlas/blob/main/arch/register_macros.hpp
@colby-nyce how do you recommend we use the register read and write macros in the instruction handler? Currently they take a string (the register name), but we don't have a method in AtlasInst to get the operand names for integer and floating point registers. We could add a method to AtlasInst to get the register number. Something like getRs1Number.
There was a problem hiding this comment.
IMO, I have only used / felt the macros to be useful when writing to CSRs so we can deal with the bitmasks automatically. But I don't see the point in using the macros for rs1/rs2/rd. What would the call site look like and how is it better than what we are using today with getRs1/dmiRead directly?
If you want to use the macros, I would probably want to add new macros like READ_RS1_VAL, READ_RD_VAL, WRITE_RD_VAL, etc.
typedef READ_RS1_VAL state->getCurrentInst()->getRs1()->dmiRead<uint64_t>()
...
But that isn't giving any real added value like with CSRs and bitmasks.
There was a problem hiding this comment.
I thought we needed the macros to control treating x0 as read-only. Is that true? Or is that handled in the Sparta register class?
If we only want to use the macros for the CSRs, then we should remove the integer and floating point macros.
I am pretty sure there are logical mistakes about types. But eyeballing and scrutinizing can only carry me as far as it is now.