Jordi Vaquero has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/20919 )
Change subject: cpu, mem: Changing AtomicOpFunctor* for
unique_ptr<AtomicOpFunctor>
......................................................................
cpu, mem: Changing AtomicOpFunctor* for unique_ptr<AtomicOpFunctor>
This change is based on modify the way we move the AtomicOpFunctor*
through gem5 in order to mantain proper ownership of the object and
ensuring its destruction when it is no longer used.
Doing that we fix at the same time a memory leak in Request.hh
where we were assigning a new AtomicOpFunctor* without destroying the
previous one.
This change creates a new type AtomicOpFunctor_ptr as a
std::unique_ptr<AtomicOpFunctor> and move its ownership as needed. Except
for its only usage when AtomicOpFunc() is called.
Change-Id: Ic516f9d8217cb1ae1f0a19500e5da0336da9fd4f
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/20919
Reviewed-by: Andreas Sandberg <andreas.sandb...@arm.com>
Maintainer: Andreas Sandberg <andreas.sandb...@arm.com>
Tested-by: kokoro <noreply+kok...@google.com>
---
M src/arch/generic/memhelpers.hh
M src/base/types.hh
M src/cpu/base_dyn_inst.hh
M src/cpu/checker/cpu.hh
M src/cpu/exec_context.hh
M src/cpu/minor/exec_context.hh
M src/cpu/minor/lsq.cc
M src/cpu/minor/lsq.hh
M src/cpu/o3/cpu.hh
M src/cpu/o3/lsq.hh
M src/cpu/o3/lsq_impl.hh
M src/cpu/simple/atomic.cc
M src/cpu/simple/atomic.hh
M src/cpu/simple/base.hh
M src/cpu/simple/exec_context.hh
M src/cpu/simple/timing.cc
M src/cpu/simple/timing.hh
M src/mem/request.hh
18 files changed, 59 insertions(+), 58 deletions(-)
Approvals:
Andreas Sandberg: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/arch/generic/memhelpers.hh b/src/arch/generic/memhelpers.hh
index 7fd4f70..fa1af26 100644
--- a/src/arch/generic/memhelpers.hh
+++ b/src/arch/generic/memhelpers.hh
@@ -125,15 +125,16 @@
template <class XC, class MemT>
Fault
amoMemAtomic(XC *xc, Trace::InstRecord *traceData, MemT &mem, Addr addr,
- Request::Flags flags, AtomicOpFunctor *amo_op)
+ Request::Flags flags, AtomicOpFunctor *_amo_op)
{
- assert(amo_op);
+ assert(_amo_op);
// mem will hold the previous value at addr after the AMO completes
memset(&mem, 0, sizeof(mem));
+ AtomicOpFunctorPtr amo_op = AtomicOpFunctorPtr(_amo_op);
Fault fault = xc->amoMem(addr, (uint8_t *)&mem, sizeof(MemT), flags,
- amo_op);
+ std::move(amo_op));
if (fault == NoFault) {
mem = TheISA::gtoh(mem);
@@ -147,10 +148,11 @@
template <class XC, class MemT>
Fault
initiateMemAMO(XC *xc, Trace::InstRecord *traceData, Addr addr, MemT& mem,
- Request::Flags flags, AtomicOpFunctor *amo_op)
+ Request::Flags flags, AtomicOpFunctor *_amo_op)
{
- assert(amo_op);
- return xc->initiateMemAMO(addr, sizeof(MemT), flags, amo_op);
+ assert(_amo_op);
+ AtomicOpFunctorPtr amo_op = AtomicOpFunctorPtr(_amo_op);
+ return xc->initiateMemAMO(addr, sizeof(MemT), flags,
std::move(amo_op));
}
#endif
diff --git a/src/base/types.hh b/src/base/types.hh
index d993845..4533094 100644
--- a/src/base/types.hh
+++ b/src/base/types.hh
@@ -259,6 +259,8 @@
virtual void execute(T * p) = 0;
};
+typedef std::unique_ptr<AtomicOpFunctor> AtomicOpFunctorPtr;
+
enum ByteOrder {
BigEndianByteOrder,
LittleEndianByteOrder
diff --git a/src/cpu/base_dyn_inst.hh b/src/cpu/base_dyn_inst.hh
index de76559..4b4b05c 100644
--- a/src/cpu/base_dyn_inst.hh
+++ b/src/cpu/base_dyn_inst.hh
@@ -311,7 +311,7 @@
const std::vector<bool>& byteEnable =
std::vector<bool>());
Fault initiateMemAMO(Addr addr, unsigned size, Request::Flags flags,
- AtomicOpFunctor *amo_op);
+ AtomicOpFunctorPtr amo_op);
/** True if the DTB address translation has started. */
bool translationStarted() const { return
instFlags[TranslationStarted]; }
@@ -986,7 +986,7 @@
Fault
BaseDynInst<Impl>::initiateMemAMO(Addr addr, unsigned size,
Request::Flags flags,
- AtomicOpFunctor *amo_op)
+ AtomicOpFunctorPtr amo_op)
{
// atomic memory instructions do not have data to be written to memory
yet
// since the atomic operations will be executed directly in
cache/memory.
@@ -995,7 +995,8 @@
// memory
return cpu->pushRequest(
dynamic_cast<typename DynInstPtr::PtrType>(this),
- /* atomic */ false, nullptr, size, addr, flags, nullptr,
amo_op);
+ /* atomic */ false, nullptr, size, addr, flags, nullptr,
+ std::move(amo_op));
}
#endif // __CPU_BASE_DYN_INST_HH__
diff --git a/src/cpu/checker/cpu.hh b/src/cpu/checker/cpu.hh
index 440fe81..8db6aa3 100644
--- a/src/cpu/checker/cpu.hh
+++ b/src/cpu/checker/cpu.hh
@@ -565,7 +565,7 @@
override;
Fault amoMem(Addr addr, uint8_t* data, unsigned size,
- Request::Flags flags, AtomicOpFunctor *amo_op) override
+ Request::Flags flags, AtomicOpFunctorPtr amo_op) override
{
panic("AMO is not supported yet in CPU checker\n");
}
diff --git a/src/cpu/exec_context.hh b/src/cpu/exec_context.hh
index b294387..a2b3924 100644
--- a/src/cpu/exec_context.hh
+++ b/src/cpu/exec_context.hh
@@ -270,7 +270,7 @@
*/
virtual Fault amoMem(Addr addr, uint8_t *data, unsigned int size,
Request::Flags flags,
- AtomicOpFunctor *amo_op)
+ AtomicOpFunctorPtr amo_op)
{
panic("ExecContext::amoMem() should be overridden\n");
}
@@ -281,7 +281,7 @@
*/
virtual Fault initiateMemAMO(Addr addr, unsigned int size,
Request::Flags flags,
- AtomicOpFunctor *amo_op)
+ AtomicOpFunctorPtr amo_op)
{
panic("ExecContext::initiateMemAMO() should be overridden\n");
}
diff --git a/src/cpu/minor/exec_context.hh b/src/cpu/minor/exec_context.hh
index 1871e24..87787f0 100644
--- a/src/cpu/minor/exec_context.hh
+++ b/src/cpu/minor/exec_context.hh
@@ -133,11 +133,11 @@
Fault
initiateMemAMO(Addr addr, unsigned int size, Request::Flags flags,
- AtomicOpFunctor *amo_op) override
+ AtomicOpFunctorPtr amo_op) override
{
// AMO requests are pushed through the store path
return execute.getLSQ().pushRequest(inst, false /* amo */, nullptr,
- size, addr, flags, nullptr, amo_op);
+ size, addr, flags, nullptr, std::move(amo_op));
}
RegVal
diff --git a/src/cpu/minor/lsq.cc b/src/cpu/minor/lsq.cc
index 1e5e896..629d89d 100644
--- a/src/cpu/minor/lsq.cc
+++ b/src/cpu/minor/lsq.cc
@@ -1573,7 +1573,7 @@
Fault
LSQ::pushRequest(MinorDynInstPtr inst, bool isLoad, uint8_t *data,
unsigned int size, Addr addr, Request::Flags flags,
- uint64_t *res, AtomicOpFunctor *amo_op,
+ uint64_t *res, AtomicOpFunctorPtr amo_op,
const std::vector<bool>& byteEnable)
{
assert(inst->translationFault == NoFault || inst->inLSQ);
@@ -1635,7 +1635,7 @@
request->request->setVirt(0 /* asid */,
addr, size, flags, cpu.dataMasterId(),
/* I've no idea why we need the PC, but give it */
- inst->pc.instAddr(), amo_op);
+ inst->pc.instAddr(), std::move(amo_op));
request->request->setByteEnable(byteEnable);
requests.push(request);
diff --git a/src/cpu/minor/lsq.hh b/src/cpu/minor/lsq.hh
index a7c7cb6..c4baad8 100644
--- a/src/cpu/minor/lsq.hh
+++ b/src/cpu/minor/lsq.hh
@@ -708,7 +708,7 @@
* the LSQ */
Fault pushRequest(MinorDynInstPtr inst, bool isLoad, uint8_t *data,
unsigned int size, Addr addr, Request::Flags flags,
- uint64_t *res, AtomicOpFunctor *amo_op,
+ uint64_t *res, AtomicOpFunctorPtr amo_op,
const std::vector<bool>& byteEnable =
std::vector<bool>());
diff --git a/src/cpu/o3/cpu.hh b/src/cpu/o3/cpu.hh
index ac917db..b06182d 100644
--- a/src/cpu/o3/cpu.hh
+++ b/src/cpu/o3/cpu.hh
@@ -713,13 +713,13 @@
/** CPU pushRequest function, forwards request to LSQ. */
Fault pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data,
unsigned int size, Addr addr, Request::Flags flags,
- uint64_t *res, AtomicOpFunctor *amo_op = nullptr,
+ uint64_t *res, AtomicOpFunctorPtr amo_op = nullptr,
const std::vector<bool>& byteEnable =
std::vector<bool>())
{
return iew.ldstQueue.pushRequest(inst, isLoad, data, size, addr,
- flags, res, amo_op, byteEnable);
+ flags, res, std::move(amo_op), byteEnable);
}
/** CPU read function, forwards read to LSQ. */
diff --git a/src/cpu/o3/lsq.hh b/src/cpu/o3/lsq.hh
index cc14ae4..6225c50 100644
--- a/src/cpu/o3/lsq.hh
+++ b/src/cpu/o3/lsq.hh
@@ -299,7 +299,7 @@
const Request::Flags _flags;
std::vector<bool> _byteEnable;
uint32_t _numOutstandingPackets;
- AtomicOpFunctor *_amo_op;
+ AtomicOpFunctorPtr _amo_op;
protected:
LSQUnit* lsqUnit() { return &_port; }
LSQRequest(LSQUnit* port, const DynInstPtr& inst, bool isLoad) :
@@ -318,7 +318,7 @@
const Addr& addr, const uint32_t& size,
const Request::Flags& flags_,
PacketDataPtr data = nullptr, uint64_t* res = nullptr,
- AtomicOpFunctor* amo_op = nullptr)
+ AtomicOpFunctorPtr amo_op = nullptr)
: _state(State::NotIssued), _senderState(nullptr),
numTranslatedFragments(0),
numInTranslationFragments(0),
@@ -326,7 +326,7 @@
_res(res), _addr(addr), _size(size),
_flags(flags_),
_numOutstandingPackets(0),
- _amo_op(amo_op)
+ _amo_op(std::move(amo_op))
{
flags.set(Flag::IsLoad, isLoad);
flags.set(Flag::WbStore,
@@ -412,7 +412,8 @@
isAnyActiveElement(byteEnable.begin(), byteEnable.end())) {
auto request = std::make_shared<Request>(_inst->getASID(),
addr, size, _flags, _inst->masterId(),
- _inst->instAddr(), _inst->contextId(), _amo_op);
+ _inst->instAddr(), _inst->contextId(),
+ std::move(_amo_op));
if (!byteEnable.empty()) {
request->setByteEnable(byteEnable);
}
@@ -721,9 +722,9 @@
const Request::Flags& flags_,
PacketDataPtr data = nullptr,
uint64_t* res = nullptr,
- AtomicOpFunctor* amo_op = nullptr) :
+ AtomicOpFunctorPtr amo_op = nullptr) :
LSQRequest(port, inst, isLoad, addr, size, flags_, data, res,
- amo_op) {}
+ std::move(amo_op)) {}
inline virtual ~SingleDataRequest() {}
virtual void initiateTranslation();
@@ -1032,7 +1033,7 @@
Fault pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data,
unsigned int size, Addr addr, Request::Flags flags,
- uint64_t *res, AtomicOpFunctor *amo_op,
+ uint64_t *res, AtomicOpFunctorPtr amo_op,
const std::vector<bool>& byteEnable);
/** The CPU pointer. */
diff --git a/src/cpu/o3/lsq_impl.hh b/src/cpu/o3/lsq_impl.hh
index e885e61..c2d5e90 100644
--- a/src/cpu/o3/lsq_impl.hh
+++ b/src/cpu/o3/lsq_impl.hh
@@ -687,7 +687,7 @@
Fault
LSQ<Impl>::pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data,
unsigned int size, Addr addr, Request::Flags flags,
- uint64_t *res, AtomicOpFunctor *amo_op,
+ uint64_t *res, AtomicOpFunctorPtr amo_op,
const std::vector<bool>& byteEnable)
{
// This comming request can be either load, store or atomic.
@@ -717,7 +717,7 @@
size, flags, data, res);
} else {
req = new SingleDataRequest(&thread[tid], inst, isLoad, addr,
- size, flags, data, res, amo_op);
+ size, flags, data, res, std::move(amo_op));
}
assert(req);
if (!byteEnable.empty()) {
diff --git a/src/cpu/simple/atomic.cc b/src/cpu/simple/atomic.cc
index a873e6d..9052cee 100644
--- a/src/cpu/simple/atomic.cc
+++ b/src/cpu/simple/atomic.cc
@@ -566,7 +566,7 @@
Fault
AtomicSimpleCPU::amoMem(Addr addr, uint8_t* data, unsigned size,
- Request::Flags flags, AtomicOpFunctor *amo_op)
+ Request::Flags flags, AtomicOpFunctorPtr amo_op)
{
SimpleExecContext& t_info = *threadInfo[curThread];
SimpleThread* thread = t_info.thread;
@@ -596,7 +596,7 @@
req->taskId(taskId());
req->setVirt(0, addr, size, flags, dataMasterId(),
- thread->pcState().instAddr(), amo_op);
+ thread->pcState().instAddr(), std::move(amo_op));
// translate to physical address
Fault fault = thread->dtb->translateAtomic(req, thread->getTC(),
diff --git a/src/cpu/simple/atomic.hh b/src/cpu/simple/atomic.hh
index 69ac09e..121cecd 100644
--- a/src/cpu/simple/atomic.hh
+++ b/src/cpu/simple/atomic.hh
@@ -227,7 +227,7 @@
override;
Fault amoMem(Addr addr, uint8_t* data, unsigned size,
- Request::Flags flags, AtomicOpFunctor *amo_op) override;
+ Request::Flags flags, AtomicOpFunctorPtr amo_op) override;
void regProbePoints() override;
diff --git a/src/cpu/simple/base.hh b/src/cpu/simple/base.hh
index 5404e5d..f8e534c 100644
--- a/src/cpu/simple/base.hh
+++ b/src/cpu/simple/base.hh
@@ -162,12 +162,12 @@
virtual Fault amoMem(Addr addr, uint8_t* data, unsigned size,
Request::Flags flags,
- AtomicOpFunctor *amo_op)
+ AtomicOpFunctorPtr amo_op)
{ panic("amoMem() is not implemented\n"); }
virtual Fault initiateMemAMO(Addr addr, unsigned size,
Request::Flags flags,
- AtomicOpFunctor *amo_op)
+ AtomicOpFunctorPtr amo_op)
{ panic("initiateMemAMO() is not implemented\n"); }
void countInst();
diff --git a/src/cpu/simple/exec_context.hh b/src/cpu/simple/exec_context.hh
index de98d6e..91f7ec5 100644
--- a/src/cpu/simple/exec_context.hh
+++ b/src/cpu/simple/exec_context.hh
@@ -463,16 +463,16 @@
}
Fault amoMem(Addr addr, uint8_t *data, unsigned int size,
- Request::Flags flags, AtomicOpFunctor *amo_op) override
+ Request::Flags flags, AtomicOpFunctorPtr amo_op) override
{
- return cpu->amoMem(addr, data, size, flags, amo_op);
+ return cpu->amoMem(addr, data, size, flags, std::move(amo_op));
}
Fault initiateMemAMO(Addr addr, unsigned int size,
Request::Flags flags,
- AtomicOpFunctor *amo_op) override
+ AtomicOpFunctorPtr amo_op) override
{
- return cpu->initiateMemAMO(addr, size, flags, amo_op);
+ return cpu->initiateMemAMO(addr, size, flags, std::move(amo_op));
}
/**
diff --git a/src/cpu/simple/timing.cc b/src/cpu/simple/timing.cc
index 4aa008e..d05eece 100644
--- a/src/cpu/simple/timing.cc
+++ b/src/cpu/simple/timing.cc
@@ -564,7 +564,7 @@
Fault
TimingSimpleCPU::initiateMemAMO(Addr addr, unsigned size,
Request::Flags flags,
- AtomicOpFunctor *amo_op)
+ AtomicOpFunctorPtr amo_op)
{
SimpleExecContext &t_info = *threadInfo[curThread];
SimpleThread* thread = t_info.thread;
@@ -579,7 +579,8 @@
traceData->setMem(addr, size, flags);
RequestPtr req = make_shared<Request>(asid, addr, size, flags,
- dataMasterId(), pc, thread->contextId(),
amo_op);
+ dataMasterId(), pc, thread->contextId(),
+ std::move(amo_op));
assert(req->hasAtomicOpFunctor());
diff --git a/src/cpu/simple/timing.hh b/src/cpu/simple/timing.hh
index 53e0ed7..27faa17 100644
--- a/src/cpu/simple/timing.hh
+++ b/src/cpu/simple/timing.hh
@@ -293,7 +293,7 @@
override;
Fault initiateMemAMO(Addr addr, unsigned size, Request::Flags flags,
- AtomicOpFunctor *amo_op) override;
+ AtomicOpFunctorPtr amo_op) override;
void fetch();
void sendFetch(const Fault &fault,
diff --git a/src/mem/request.hh b/src/mem/request.hh
index 324ae38..5094493 100644
--- a/src/mem/request.hh
+++ b/src/mem/request.hh
@@ -389,7 +389,7 @@
InstSeqNum _reqInstSeqNum;
/** A pointer to an atomic operation */
- AtomicOpFunctor *atomicOpFunctor;
+ AtomicOpFunctorPtr atomicOpFunctor;
public:
@@ -470,9 +470,9 @@
Request(uint64_t asid, Addr vaddr, unsigned size, Flags flags,
MasterID mid, Addr pc, ContextID cid,
- AtomicOpFunctor *atomic_op)
+ AtomicOpFunctorPtr atomic_op)
{
- setVirt(asid, vaddr, size, flags, mid, pc, atomic_op);
+ setVirt(asid, vaddr, size, flags, mid, pc, std::move(atomic_op));
setContext(cid);
}
@@ -489,18 +489,12 @@
translateDelta(other.translateDelta),
accessDelta(other.accessDelta), depth(other.depth)
{
- if (other.atomicOpFunctor)
- atomicOpFunctor = (other.atomicOpFunctor)->clone();
- else
- atomicOpFunctor = nullptr;
+
+ atomicOpFunctor.reset(other.atomicOpFunctor ?
+ other.atomicOpFunctor->clone() : nullptr);
}
- ~Request()
- {
- if (hasAtomicOpFunctor()) {
- delete atomicOpFunctor;
- }
- }
+ ~Request() {}
/**
* Set up Context numbers.
@@ -533,7 +527,7 @@
*/
void
setVirt(uint64_t asid, Addr vaddr, unsigned size, Flags flags,
- MasterID mid, Addr pc, AtomicOpFunctor *amo_op = nullptr)
+ MasterID mid, Addr pc, AtomicOpFunctorPtr amo_op = nullptr)
{
_asid = asid;
_vaddr = vaddr;
@@ -549,7 +543,7 @@
depth = 0;
accessDelta = 0;
translateDelta = 0;
- atomicOpFunctor = amo_op;
+ atomicOpFunctor = std::move(amo_op);
}
/**
@@ -669,14 +663,14 @@
bool
hasAtomicOpFunctor()
{
- return atomicOpFunctor != NULL;
+ return (bool)atomicOpFunctor;
}
AtomicOpFunctor *
getAtomicOpFunctor()
{
- assert(atomicOpFunctor != NULL);
- return atomicOpFunctor;
+ assert(atomicOpFunctor);
+ return atomicOpFunctor.get();
}
/** Accessor for flags. */
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/20919
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: Ic516f9d8217cb1ae1f0a19500e5da0336da9fd4f
Gerrit-Change-Number: 20919
Gerrit-PatchSet: 7
Gerrit-Owner: Jordi Vaquero <jordi.vaqu...@metempsy.com>
Gerrit-Reviewer: Andreas Sandberg <andreas.sandb...@arm.com>
Gerrit-Reviewer: Ciro Santilli <ciro.santi...@arm.com>
Gerrit-Reviewer: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-Reviewer: Jason Lowe-Power <ja...@lowepower.com>
Gerrit-Reviewer: Jordi Vaquero <jordi.vaqu...@metempsy.com>
Gerrit-Reviewer: Tuan Ta <q...@cornell.edu>
Gerrit-Reviewer: kokoro <noreply+kok...@google.com>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev
