Hi Nate,
I noticed that these changes introduced a couple bugs. I believe the
Packet constructors should set the VALID_DST flag and the logic of the
allocate function is incorrect. I believe we should make the following
changes to packet.hh. Would you like me to go ahead and update the
m5-dev repository?
Brad
diff -r 4f5d99098862 src/mem/packet.hh
--- a/src/mem/packet.hh Tue Nov 11 11:41:19 2008 -0800
+++ b/src/mem/packet.hh Wed Nov 12 16:31:55 2008 -0800
@@ -461,8 +461,8 @@ class Packet : public FastAlloc, public
* supplied.
*/
Packet(Request *_req, MemCmd _cmd, NodeID _dest)
- : cmd(_cmd), req(_req), data(NULL), addr(_req->paddr),
- size(_req->size), dest(_dest), time(curTick),
senderState(NULL)
+ : flags(VALID_DST), cmd(_cmd), req(_req), data(NULL),
addr(_req->paddr),
+ size(_req->size), dest(_dest), time(curTick),
senderState(NULL)
{
}
@@ -472,7 +472,7 @@ class Packet : public FastAlloc, public
* req. this allows for overriding the size/addr of the req.
*/
Packet(Request *_req, MemCmd _cmd, NodeID _dest, int _blkSize)
- : cmd(_cmd), req(_req), data(NULL),
+ : flags(VALID_DST), cmd(_cmd), req(_req), data(NULL),
addr(_req->paddr & ~(_blkSize - 1)), size(_blkSize),
dest(_dest),
time(curTick), senderState(NULL)
{
@@ -701,12 +701,11 @@ class Packet : public FastAlloc, public
void
allocate()
{
- if (data) {
- assert(flags.none(STATIC_DATA|DYNAMIC_DATA));
- } else {
- flags.set(DYNAMIC_DATA|ARRAY_DATA);
- data = new uint8_t[getSize()];
- }
+ if (data)
+ return;
+ assert(flags.none(STATIC_DATA));
+ flags.set(DYNAMIC_DATA|ARRAY_DATA);
+ data = new uint8_t[getSize()];
}
> -----Original Message-----
> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On
> Behalf Of nathan binkert
> Sent: Monday, November 10, 2008 6:55 PM
> To: m5-dev@m5sim.org
> Subject: Re: [m5-dev] changeset in m5: style: clean up the Packet
stuff
>
> This diff went way beyond style cleanup. Unfortunately, I didn't
> remember to update the commit message. One of the big things I did
> was get rid of all of the individual boolean variable flags and create
> a single flags variable. I also made all of the flag values static
> members of their classes instead of globals.
>
> Nate
>
> On Mon, Nov 10, 2008 at 6:49 PM, Nathan Binkert <[EMAIL PROTECTED]>
> wrote:
> > changeset a88e8e7dec75 in /z/repo/m5
> > details: http://repo.m5sim.org/m5?cmd=changeset;node=a88e8e7dec75
> > description:
> > style: clean up the Packet stuff
> >
> > diffstat:
> >
> > 3 files changed, 24 insertions(+), 32 deletions(-)
> > src/mem/packet.cc | 1
> > src/mem/packet.hh | 1
> > src/mem/request.hh | 54
+++++++++++++++++++++----------------------
> ---------
> >
> > diffs (truncated from 1501 to 300 lines):
> >
> > diff -r f3733e2b19d5 -r a88e8e7dec75 src/mem/packet.cc
> > --- a/src/mem/packet.cc Mon Nov 10 11:51:17 2008 -0800
> > +++ b/src/mem/packet.cc Mon Nov 10 11:51:17 2008 -0800
> > @@ -41,6 +41,8 @@
> > #include "base/misc.hh"
> > #include "base/trace.hh"
> > #include "mem/packet.hh"
> > +
> > +using namespace std;
> >
> > // The one downside to bitsets is that static initializers can get
> ugly.
> > #define SET1(a1) (1 << (a1))
> > @@ -133,35 +135,6 @@
> > { SET2(IsRequest, IsPrint), InvalidCmd, "PrintReq" }
> > };
> >
> > -
> > -/** delete the data pointed to in the data pointer. Ok to call to
> matter how
> > - * data was allocted. */
> > -void
> > -Packet::deleteData()
> > -{
> > - assert(staticData || dynamicData);
> > - if (staticData)
> > - return;
> > -
> > - if (arrayData)
> > - delete [] data;
> > - else
> > - delete data;
> > -}
> > -
> > -/** If there isn't data in the packet, allocate some. */
> > -void
> > -Packet::allocate()
> > -{
> > - if (data)
> > - return;
> > - assert(!staticData);
> > - dynamicData = true;
> > - arrayData = true;
> > - data = new uint8_t[getSize()];
> > -}
> > -
> > -
> > bool
> > Packet::checkFunctional(Printable *obj, Addr addr, int size,
uint8_t
> *data)
> > {
> > @@ -193,7 +166,7 @@
> > if (isRead()) {
> > if (func_start >= val_start && func_end <= val_end) {
> > allocate();
> > - std::memcpy(getPtr<uint8_t>(), data + offset,
> getSize());
> > + memcpy(getPtr<uint8_t>(), data + offset, getSize());
> > makeResponse();
> > return true;
> > } else {
> > @@ -208,11 +181,12 @@
> > }
> > } else if (isWrite()) {
> > if (offset >= 0) {
> > - std::memcpy(data + offset, getPtr<uint8_t>(),
> > - (std::min(func_end, val_end) - func_start)
+
> 1);
> > - } else { // val_start > func_start
> > - std::memcpy(data, getPtr<uint8_t>() - offset,
> > - (std::min(func_end, val_end) - val_start) +
> 1);
> > + memcpy(data + offset, getPtr<uint8_t>(),
> > + (min(func_end, val_end) - func_start) + 1);
> > + } else {
> > + // val_start > func_start
> > + memcpy(data, getPtr<uint8_t>() - offset,
> > + (min(func_end, val_end) - val_start) + 1);
> > }
> > } else {
> > panic("Don't know how to handle command %s\n", cmdString());
> > @@ -222,22 +196,18 @@
> > return false;
> > }
> >
> > -
> > void
> > -Packet::print(std::ostream &o, const int verbosity,
> > - const std::string &prefix) const
> > +Packet::print(ostream &o, const int verbosity, const string
&prefix)
> const
> > {
> > ccprintf(o, "%s[%x:%x] %s\n", prefix,
> > getAddr(), getAddr() + getSize() - 1, cmdString());
> > }
> >
> > -
> > -Packet::PrintReqState::PrintReqState(std::ostream &_os, int
> _verbosity)
> > - : curPrefixPtr(new std::string("")), os(_os),
> verbosity(_verbosity)
> > +Packet::PrintReqState::PrintReqState(ostream &_os, int _verbosity)
> > + : curPrefixPtr(new string("")), os(_os), verbosity(_verbosity)
> > {
> > labelStack.push_back(LabelStackEntry("", curPrefixPtr));
> > }
> > -
> >
> > Packet::PrintReqState::~PrintReqState()
> > {
> > @@ -246,21 +216,17 @@
> > delete curPrefixPtr;
> > }
> >
> > -
> > Packet::PrintReqState::
> > -LabelStackEntry::LabelStackEntry(const std::string &_label,
> > - std::string *_prefix)
> > +LabelStackEntry::LabelStackEntry(const string &_label, string
> *_prefix)
> > : label(_label), prefix(_prefix), labelPrinted(false)
> > {
> > }
> >
> > -
> > void
> > -Packet::PrintReqState::pushLabel(const std::string &lbl,
> > - const std::string &prefix)
> > +Packet::PrintReqState::pushLabel(const string &lbl, const string
> &prefix)
> > {
> > labelStack.push_back(LabelStackEntry(lbl, curPrefixPtr));
> > - curPrefixPtr = new std::string(*curPrefixPtr);
> > + curPrefixPtr = new string(*curPrefixPtr);
> > *curPrefixPtr += prefix;
> > }
> >
> > diff -r f3733e2b19d5 -r a88e8e7dec75 src/mem/packet.hh
> > --- a/src/mem/packet.hh Mon Nov 10 11:51:17 2008 -0800
> > +++ b/src/mem/packet.hh Mon Nov 10 11:51:17 2008 -0800
> > @@ -42,8 +42,10 @@
> > #include <list>
> > #include <bitset>
> >
> > +#include "base/cast.hh"
> > #include "base/compiler.hh"
> > #include "base/fast_alloc.hh"
> > +#include "base/flags.hh"
> > #include "base/misc.hh"
> > #include "base/printable.hh"
> > #include "mem/request.hh"
> > @@ -58,9 +60,12 @@
> >
> > class MemCmd
> > {
> > + friend class Packet;
> > +
> > public:
> > -
> > - /** List of all commands associated with a packet. */
> > + /**
> > + * List of all commands associated with a packet.
> > + */
> > enum Command
> > {
> > InvalidCmd,
> > @@ -100,7 +105,9 @@
> > };
> >
> > private:
> > - /** List of command attributes. */
> > + /**
> > + * List of command attributes.
> > + */
> > enum Attribute
> > {
> > IsRead, //!< Data flows from responder to requester
> > @@ -120,26 +127,31 @@
> > NUM_COMMAND_ATTRIBUTES
> > };
> >
> > - /** Structure that defines attributes and other data associated
> > - * with a Command. */
> > - struct CommandInfo {
> > - /** Set of attribute flags. */
> > + /**
> > + * Structure that defines attributes and other data associated
> > + * with a Command.
> > + */
> > + struct CommandInfo
> > + {
> > + /// Set of attribute flags.
> > const std::bitset<NUM_COMMAND_ATTRIBUTES> attributes;
> > - /** Corresponding response for requests; InvalidCmd if no
> > - * response is applicable. */
> > + /// Corresponding response for requests; InvalidCmd if no
> > + /// response is applicable.
> > const Command response;
> > - /** String representation (for printing) */
> > + /// String representation (for printing)
> > const std::string str;
> > };
> >
> > - /** Array to map Command enum to associated info. */
> > + /// Array to map Command enum to associated info.
> > static const CommandInfo commandInfo[];
> >
> > private:
> >
> > Command cmd;
> >
> > - bool testCmdAttrib(MemCmd::Attribute attrib) const {
> > + bool
> > + testCmdAttrib(MemCmd::Attribute attrib) const
> > + {
> > return commandInfo[cmd].attributes[attrib] != 0;
> > }
> >
> > @@ -158,33 +170,22 @@
> > bool isError() const { return testCmdAttrib(IsError); }
> > bool isPrint() const { return testCmdAttrib(IsPrint); }
> >
> > - const Command responseCommand() const {
> > + const Command
> > + responseCommand() const
> > + {
> > return commandInfo[cmd].response;
> > }
> >
> > - /** Return the string to a cmd given by idx. */
> > - const std::string &toString() const {
> > - return commandInfo[cmd].str;
> > - }
> > -
> > + /// Return the string to a cmd given by idx.
> > + const std::string &toString() const { return
> commandInfo[cmd].str; }
> > int toInt() const { return (int)cmd; }
> >
> > - MemCmd(Command _cmd)
> > - : cmd(_cmd)
> > - { }
> > + MemCmd(Command _cmd) : cmd(_cmd) { }
> > + MemCmd(int _cmd) : cmd((Command)_cmd) { }
> > + MemCmd() : cmd(InvalidCmd) { }
> >
> > - MemCmd(int _cmd)
> > - : cmd((Command)_cmd)
> > - { }
> > -
> > - MemCmd()
> > - : cmd(InvalidCmd)
> > - { }
> > -
> > - bool operator==(MemCmd c2) { return (cmd == c2.cmd); }
> > - bool operator!=(MemCmd c2) { return (cmd != c2.cmd); }
> > -
> > - friend class Packet;
> > + bool operator==(MemCmd c2) const { return (cmd == c2.cmd); }
> > + bool operator!=(MemCmd c2) const { return (cmd != c2.cmd); }
> > };
> >
> > /**
> > @@ -197,107 +198,118 @@
> > class Packet : public FastAlloc, public Printable
> > {
> > public:
> > + typedef uint32_t FlagsType;
> > + typedef ::Flags<FlagsType> Flags;
> > + typedef short NodeID;
> >
> > + private:
> > + static const FlagsType PUBLIC_FLAGS = 0x00000000;
> > + static const FlagsType PRIVATE_FLAGS = 0x00007F0F;
> > + static const FlagsType COPY_FLAGS = 0x0000000F;
> > +
> > + static const FlagsType SHARED = 0x00000001;
> > + // Special control flags
> > + /// Special timing-mode atomic snoop for multi-level coherence.
> > + static const FlagsType EXPRESS_SNOOP = 0x00000002;
> > + /// Does supplier have exclusive copy?
> > + /// Useful for multi-level coherence.
> > + static const FlagsType SUPPLY_EXCLUSIVE = 0x00000004;
> > + // Snoop response flags
> > + static const FlagsType MEM_INHIBIT = 0x00000008;
> > + /// Are the 'addr' and 'size' fields valid?
> > + static const FlagsType VALID_ADDR = 0x00000100;
> > + static const FlagsType VALID_SIZE = 0x00000200;
> > + /// Is the 'src' field valid?
> > + static const FlagsType VALID_SRC = 0x00000400;
> > + static const FlagsType VALID_DST = 0x00000800;
> > + /// Is the data pointer set to a value that shouldn't be freed
> > + /// when the packet is destroyed?
> > + static const FlagsType STATIC_DATA = 0x00001000;
> > + /// The data pointer points to a value that should be freed
when
> > + /// the packet is destroyed.
> > + static const FlagsType DYNAMIC_DATA = 0x00002000;
> > + /// the data pointer points to an array (thus delete []) needs
> to
> > + /// be called on it rather than simply delete.
> > + static const FlagsType ARRAY_DATA = 0x00004000;
> > +
> > + Flags flags;
> > +
> > + public:
> > typedef MemCmd::Command Command;
> >
> > - /** The command field of the packet. */
> > + /// The command field of the packet.
> > MemCmd cmd;
> >
> > - /** A pointer to the original request. */
> > + /// A pointer to the original request.
> > _______________________________________________
> > m5-dev mailing list
> > m5-dev@m5sim.org
> > http://m5sim.org/mailman/listinfo/m5-dev
> >
> >
> _______________________________________________
> m5-dev mailing list
> m5-dev@m5sim.org
> http://m5sim.org/mailman/listinfo/m5-dev
_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
