Skip to content

Conversation

@leekillough
Copy link
Collaborator

This is cleanup I did when exhaustively reviewing PR #205 .

In compressed instructions where rs1 = rd, make the decoding code assign to both rs1 and rd in the same statement, to avoid mistakes and to prevent creating a dependency on whether rs1 or rd is initialized first (some of the previous code assigned to one and then used it in a later statement to assign to the other, which is confusing -- this makes it clear that both are being assigned the same value at once).

Rename make_lsq_hash() to LSQHash(), keeping in line with CamelCase.

Clean up MemReq class to only have a constructor, and not a .Set() function to change it after it's created (it still has an assignment operator= which is used in RevMemOp::setMemReq()). Split the constructor across multiple lines, one member per line, for readability. Directly initialize MemReq objects using this constructor, instead of default-initializing a MemReq value and using .Set() afterwards.

Since they are common operations, add LSQHash() and LSQHashPair() functions to MemReq. LSQHash() returns the namespace-scope LSQHash() on selected MemReq members, and LSQHashPair() returns {LSQHash(), *this}, a pair consisting of the hash and the request, for storing in std::unordered_map.

Make data members of MemReq private, only visible to MemReq and friend classes RevTracer, RevMem and RevProc.

Instead of MarkLoadComplete() being a member functor of MemReq which expects a MemReq argument, change MarkLoadComplete() to be a member function of MemReq which takes no arguments and calls a MarkLoadCompleteFunc functor with *this as its argument, so that req.MarkLoadComplete(req) can simply be called as req.MarkLoadComplete().

Use std::function<void(const MemReq&)> consistently throughout the code. Previously std::function<void(MemReq)> was used in some places. While it may work because of conversions, it is not as clean.

Using the new helper functions, replace the common idiom R->LSQueue->insert({make_lsq_hash(Inst.rd, RevRegClass::RegGPR, F->GetHartToExecID()), req}); with the shorter R->LSQueue->insert(req.LSQHashPair());

Add std::move() around MemReq objects the last time that they are used locally, as a minor optimization (prevents having to create deep copies of dynamically-allocated objects such as lambda captures -- allows them to be shallowly moved instead).

Make the underlying type for RevReg explicitly uint16_t, since uint16_t is the type used for register numbers in a lot of APIs.

Templatize the MemReq constructor and the LSQHash(), DependencySet() and DependencyClear() functions, so that they accept either RevReg enum class or an integer register number. Replace hard-coded 10 with RevReg::a0 and 0 with RevReg::zero.

In many .cpp files, using namespace SST::RevCPU; was used to pull in symbols, but the rest of the file still defined functions in global namespace unless they were qualified names (using ::). Change all of those files to use namespace SST::RevCPU{ ... } instead of using namespace SST::RevCPU;, so that everything in the file is defined in the SST::RevCPU namespace.

@kpgriesser
Copy link
Collaborator

Not a concern but for future consideration (or maybe this is already there). If I wanted to put in another memory agent, like a DMA, it would be nice to build it on the MemCtl/MemReq classes. The receiving agent would receive a block of data and have a custom completion. The requests could go through the same queue to the same memory port or in parallel to a separate memory port. I've been playing with this in a coprocessor memory agent sharing the same lsq. Thanks much

@leekillough
Copy link
Collaborator Author

Not a concern but for future consideration (or maybe this is already there). If I wanted to put in another memory agent, like a DMA, it would be nice to build it on the MemCtl/MemReq classes.

That seems a bigger scope than the MemReq class, which is intended to be very lightweight and is used to implement MemHierarchy and track outstanding requests. The containers (e.g. std::unordered_map) were carefully chosen and tuned. Expanding MemReq's scope would make this run much slower.

Copy link
Collaborator

@rkabrick rkabrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants