derek/gem5
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

cpu, mem: Changing AtomicOpFunctor* for unique_ptr<AtomicOpFunctor>

Browse Source 
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.sandberg@arm.com>
Maintainer: Andreas Sandberg <andreas.sandberg@arm.com>
Tested-by: kokoro <noreply+kokoro@google.com>
This commit is contained in:
Jordi Vaquero
2019-09-11 00:11:27 +02:00
parent a56ab04598
commit e5a82da26e
18 changed files with 59 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
src/arch/generic/memhelpers.hh
View File

@@ -125,15 +125,16 @@ writeMemAtomic(XC *xc, Trace::InstRecord *traceData, const MemT &mem,
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 @@ amoMemAtomic(XC *xc, Trace::InstRecord *traceData, MemT &mem, Addr addr,
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

2
src/base/types.hh
View File

@@ -259,6 +259,8 @@ struct TypedAtomicOpFunctor : public AtomicOpFunctor
virtual void execute(T * p) = 0;
};
typedef std::unique_ptr<AtomicOpFunctor> AtomicOpFunctorPtr;
enum ByteOrder {
BigEndianByteOrder,
LittleEndianByteOrder

7
src/cpu/base_dyn_inst.hh
View File

@@ -311,7 +311,7 @@ class BaseDynInst : public ExecContext, public RefCounted
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 @@ template<class Impl>
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 @@ BaseDynInst<Impl>::initiateMemAMO(Addr addr, unsigned size,
// 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__

2
src/cpu/checker/cpu.hh
View File

@@ -565,7 +565,7 @@ class CheckerCPU : public BaseCPU, public ExecContext
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");
}

4
src/cpu/exec_context.hh
View File

@@ -270,7 +270,7 @@ class ExecContext {
*/
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 @@ class ExecContext {
*/
virtual Fault initiateMemAMO(Addr addr, unsigned int size,
Request::Flags flags,
AtomicOpFunctor *amo_op)
AtomicOpFunctorPtr amo_op)
{
panic("ExecContext::initiateMemAMO() should be overridden\n");
}

4
src/cpu/minor/exec_context.hh
View File

@@ -133,11 +133,11 @@ class ExecContext : public ::ExecContext
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

4
src/cpu/minor/lsq.cc
View File

@@ -1573,7 +1573,7 @@ LSQ::needsToTick()
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 @@ LSQ::pushRequest(MinorDynInstPtr inst, bool isLoad, uint8_t *data,
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);

2
src/cpu/minor/lsq.hh
View File

@@ -708,7 +708,7 @@ class LSQ : public Named
* 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>());

4
src/cpu/o3/cpu.hh
View File

@@ -713,13 +713,13 @@ class FullO3CPU : public BaseO3CPU
/** 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. */

15
src/cpu/o3/lsq.hh
View File

@@ -299,7 +299,7 @@ class LSQ
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 @@ class LSQ
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 @@ class LSQ
_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 @@ class LSQ
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 @@ class LSQ
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 @@ class 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,
uint64_t *res, AtomicOpFunctorPtr amo_op,
const std::vector<bool>& byteEnable);
/** The CPU pointer. */

4
src/cpu/o3/lsq_impl.hh
View File

@@ -687,7 +687,7 @@ template<class Impl>
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 @@ LSQ<Impl>::pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data,
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()) {

4
src/cpu/simple/atomic.cc
View File

@@ -566,7 +566,7 @@ AtomicSimpleCPU::writeMem(uint8_t *data, unsigned size, Addr addr,
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 @@ AtomicSimpleCPU::amoMem(Addr addr, uint8_t* data, unsigned size,
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(),

2
src/cpu/simple/atomic.hh
View File

@@ -227,7 +227,7 @@ class AtomicSimpleCPU : public BaseSimpleCPU
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;

4
src/cpu/simple/base.hh
View File

@@ -162,12 +162,12 @@ class BaseSimpleCPU : public BaseCPU
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();

8
src/cpu/simple/exec_context.hh
View File

@@ -463,16 +463,16 @@ class SimpleExecContext : public ExecContext {
}
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));
}
/**

5
src/cpu/simple/timing.cc
View File

@@ -564,7 +564,7 @@ TimingSimpleCPU::writeMem(uint8_t *data, unsigned size,
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 @@ TimingSimpleCPU::initiateMemAMO(Addr addr, unsigned size,
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());

2
src/cpu/simple/timing.hh
View File

@@ -293,7 +293,7 @@ class TimingSimpleCPU : public BaseSimpleCPU
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,

30
src/mem/request.hh
View File

@@ -389,7 +389,7 @@ class Request
InstSeqNum _reqInstSeqNum;
/** A pointer to an atomic operation */
AtomicOpFunctor *atomicOpFunctor;
AtomicOpFunctorPtr atomicOpFunctor;
public:
@@ -470,9 +470,9 @@ class Request
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 @@ class Request
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 @@ class Request
*/
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 @@ class Request
depth = 0;
accessDelta = 0;
translateDelta = 0;
atomicOpFunctor = amo_op;
atomicOpFunctor = std::move(amo_op);
}
/**
@@ -669,14 +663,14 @@ class Request
bool
hasAtomicOpFunctor()
{
return atomicOpFunctor != NULL;
return (bool)atomicOpFunctor;
}
AtomicOpFunctor *
getAtomicOpFunctor()
{
assert(atomicOpFunctor != NULL);
return atomicOpFunctor;
assert(atomicOpFunctor);
return atomicOpFunctor.get();
}
/** Accessor for flags. */