Hi Tony, It is definitely not an ideal solution, and it will break compatibility with all existing traces (trust me, this is a _big_ deal). If we stick to the uint32_t and can shift these flags to the ‘end’ of the list it is probably ok as a stop-gap solution. Please ensure it stops at the two flags though, and I would suggest also adding a @todo and comment regarding this not being final.
Andreas On 27/08/2015 10:09, "gem5-dev on behalf of Gutierrez, Anthony" <[email protected] on behalf of [email protected]> wrote: >Hi Andreas, > >While we agree there should be a better long term solution, this is what >we have now and what our GPU model - that we plan to release publicly >very soon - relies upon. If we added the flags to currently unused bits >in the flags and kept the flag size to 32 bits, would you mind if we >added these fields to the request flags until a better solution can be >agreed upon? > >-Tony >________________________________________ >From: gem5-dev [[email protected]] on behalf of Andreas Hansson >[[email protected]] >Sent: Thursday, August 27, 2015 12:58 AM >To: gem5 Developer List; [email protected] >Subject: Re: [gem5-dev] changeset in gem5: mem: Remove extraneous >acquire/release flags ... > >Hi Brad, > >I do not think we should check this patch in as is (hence the removal). >There needs to be a sensible design around the use of flags, and I remain >to be convinced that adding a request flag for every ‘action' is not the >way to go. There is already a long discussion thread on the dev list. >Perhaps the request needs something like an Action or Command. > >Andreas > > >On 26/08/2015 23:40, "gem5-dev on behalf of Beckmann, Brad" ><[email protected] on behalf of [email protected]> wrote: > >>Andreas, >> >>It just came to my attention that this changeset removes acquire and >>release attributes from both the packet and the request. The comment on >>the checkin seemed to indicate that it was only extraneous flags, but >>these are required to make our GPU code work. We would prefer at least >>having the flags part of the request. >> >>Can we please check this patch in and keep it in the repo? >> >>Thanks, >> >>Brad >> >> >>-----Original Message----- >>From: gem5-dev [mailto:[email protected]] On Behalf Of Andreas >>Hansson >>Sent: Friday, August 07, 2015 1:56 AM >>To: [email protected] >>Subject: [gem5-dev] changeset in gem5: mem: Remove extraneous >>acquire/release flags ... >> >>changeset ba91725c8f6b in /z/repo/gem5 >>details: http://repo.gem5.org/gem5?cmd=changeset;node=ba91725c8f6b >>description: >> mem: Remove extraneous acquire/release flags and attributes >> >> This patch removes the extraneous flags and attributes from the >> request and packet, and simply leaves the new commands. The change >> introduced when adding acquire/release breaks all compatibility >>with >> existing traces, and there is really no need for any new flags and >> attributes. The commands should be sufficient. >> >> This patch fixes packet tracing (urgent), and also removes the >> unnecessary complexity. >> >>diffstat: >> >> src/mem/packet.cc | 8 ++++---- >> src/mem/packet.hh | 6 ------ >> src/mem/request.hh | 53 >>+++++++++++++++++++++-------------------------------- >> 3 files changed, 25 insertions(+), 42 deletions(-) >> >>diffs (181 lines): >> >>diff -r 4413195267fd -r ba91725c8f6b src/mem/packet.cc >>--- a/src/mem/packet.cc Wed Aug 05 10:27:11 2015 +0100 >>+++ b/src/mem/packet.cc Fri Aug 07 04:55:38 2015 -0400 >>@@ -166,13 +166,13 @@ >> /* IntResp -- for interrupts */ >> { SET2(IsWrite, IsResponse), InvalidCmd, "MessageResp" }, >> /* ReleaseReq -- for release synchronization */ >>- { SET3(IsRelease, IsRequest, NeedsResponse), ReleaseResp, >>"ReleaseReq" }, >>+ { SET2(IsRequest, NeedsResponse), ReleaseResp, "ReleaseReq" }, >> /* ReleaseResp -- for release synchronization */ >>- { SET2(IsRelease, IsResponse), InvalidCmd, "ReleaseResp" }, >>+ { SET1(IsResponse), InvalidCmd, "ReleaseResp" }, >> /* AcquireReq -- for release synchronization */ >>- { SET3(IsAcquire, IsRequest, NeedsResponse), AcquireResp, >>"AcquireReq" }, >>+ { SET2(IsRequest, NeedsResponse), AcquireResp, "AcquireReq" }, >> /* AcquireResp -- for release synchronization */ >>- { SET3(IsAcquire, IsResponse, NeedsResponse), InvalidCmd, >>"AcquireResp" }, >>+ { SET2(IsResponse, NeedsResponse), InvalidCmd, "AcquireResp" }, >> /* InvalidDestError -- packet dest field invalid */ >> { SET2(IsResponse, IsError), InvalidCmd, "InvalidDestError" }, >> /* BadAddressError -- memory address invalid */ >>diff -r 4413195267fd -r ba91725c8f6b src/mem/packet.hh >>--- a/src/mem/packet.hh Wed Aug 05 10:27:11 2015 +0100 >>+++ b/src/mem/packet.hh Fri Aug 07 04:55:38 2015 -0400 >>@@ -151,8 +151,6 @@ >> IsError, //!< Error response >> IsPrint, //!< Print state matching address (for >>debugging) >> IsFlush, //!< Flush the address from caches >>- IsAcquire, //!< Acquire operation >>- IsRelease, //!< Release operation >> NUM_COMMAND_ATTRIBUTES >> }; >> >>@@ -209,8 +207,6 @@ >> bool isError() const { return testCmdAttrib(IsError); } >> bool isPrint() const { return testCmdAttrib(IsPrint); } >> bool isFlush() const { return testCmdAttrib(IsFlush); } >>- bool isAcquire() const { return testCmdAttrib(IsAcquire); } >>- bool isRelease() const { return testCmdAttrib(IsRelease); } >> >> const Command >> responseCommand() const >>@@ -492,8 +488,6 @@ >> bool isError() const { return cmd.isError(); } >> bool isPrint() const { return cmd.isPrint(); } >> bool isFlush() const { return cmd.isFlush(); } >>- bool isAcquire() const { return cmd.isAcquire(); } >>- bool isRelease() const { return cmd.isRelease(); } >> >> // Snoop flags >> void assertMemInhibit() >>diff -r 4413195267fd -r ba91725c8f6b src/mem/request.hh >>--- a/src/mem/request.hh Wed Aug 05 10:27:11 2015 +0100 >>+++ b/src/mem/request.hh Fri Aug 07 04:55:38 2015 -0400 >>@@ -86,7 +86,7 @@ >> class Request >> { >> public: >>- typedef uint64_t FlagsType; >>+ typedef uint32_t FlagsType; >> typedef uint8_t ArchFlagsType; >> typedef ::Flags<FlagsType> Flags; >> >>@@ -98,11 +98,11 @@ >> * architecture-specific code. For example, SPARC uses them to >> * represent ASIs. >> */ >>- ARCH_BITS = 0x00000000000000FF, >>+ ARCH_BITS = 0x000000FF, >> /** The request was an instruction fetch. */ >>- INST_FETCH = 0x0000000000000100, >>+ INST_FETCH = 0x00000100, >> /** The virtual address is also the physical address. */ >>- PHYSICAL = 0x0000000000000200, >>+ PHYSICAL = 0x00000200, >> /** >> * The request is to an uncacheable address. >> * >>@@ -110,7 +110,7 @@ >> * STRICT_ORDER flag should be set if such reordering is >> * undesirable. >> */ >>- UNCACHEABLE = 0x0000000000000400, >>+ UNCACHEABLE = 0x00000400, >> /** >> * The request is required to be strictly ordered by <i>CPU >> * models</i> and is non-speculative. >>@@ -120,22 +120,22 @@ >> * memory system may still reorder requests in caches unless >> * the UNCACHEABLE flag is set as well. >> */ >>- STRICT_ORDER = 0x0000000000000800, >>+ STRICT_ORDER = 0x00000800, >> /** This request is to a memory mapped register. */ >>- MMAPPED_IPR = 0x0000000000001000, >>+ MMAPPED_IPR = 0x00002000, >> /** This request is a clear exclusive. */ >>- CLEAR_LL = 0x0000000000002000, >>+ CLEAR_LL = 0x00004000, >> /** This request is made in privileged mode. */ >>- PRIVILEGED = 0x0000000000004000, >>+ PRIVILEGED = 0x00008000, >> >> /** >> * This is a write that is targeted and zeroing an entire >> * cache block. There is no need for a read/modify/write >> */ >>- CACHE_BLOCK_ZERO = 0x0000000000008000, >>+ CACHE_BLOCK_ZERO = 0x00010000, >> >> /** The request should not cause a memory access. */ >>- NO_ACCESS = 0x0000000000100000, >>+ NO_ACCESS = 0x00080000, >> /** >> * This request will lock or unlock the accessed memory. When >> * used with a load, the access locks the particular chunk of @@ >>-143,34 +143,30 @@ >> * that locked accesses have to be made up of a locked load, >> * some operation on the data, and then a locked store. >> */ >>- LOCKED_RMW = 0x0000000000200000, >>+ LOCKED_RMW = 0x00100000, >> /** The request is a Load locked/store conditional. */ >>- LLSC = 0x0000000000400000, >>+ LLSC = 0x00200000, >> /** This request is for a memory swap. */ >>- MEM_SWAP = 0x0000000000800000, >>- MEM_SWAP_COND = 0x0000000001000000, >>+ MEM_SWAP = 0x00400000, >>+ MEM_SWAP_COND = 0x00800000, >> >> /** The request is a prefetch. */ >>- PREFETCH = 0x0000000002000000, >>+ PREFETCH = 0x01000000, >> /** The request should be prefetched into the exclusive state. >>*/ >>- PF_EXCLUSIVE = 0x0000000004000000, >>+ PF_EXCLUSIVE = 0x02000000, >> /** The request should be marked as LRU. */ >>- EVICT_NEXT = 0x0000000008000000, >>- /** The request should be marked with ACQUIRE. */ >>- ACQUIRE = 0x0000000001000000, >>- /** The request should be marked with RELEASE. */ >>- RELEASE = 0x0000000002000000, >>+ EVICT_NEXT = 0x04000000, >> >> /** >> * The request should be handled by the generic IPR code (only >> * valid together with MMAPPED_IPR) >> */ >>- GENERIC_IPR = 0x0000000004000000, >>+ GENERIC_IPR = 0x08000000, >> >> /** The request targets the secure memory space. */ >>- SECURE = 0x0000000008000000, >>+ SECURE = 0x10000000, >> /** The request is a page table walk */ >>- PT_WALK = 0x0000000010000000, >>+ PT_WALK = 0x20000000, >> >> /** >> * These flags are *not* cleared when a Request object is @@ >>-660,19 +656,12 @@ >> bool isLLSC() const { return _flags.isSet(LLSC); } >> bool isPriv() const { return _flags.isSet(PRIVILEGED); } >> bool isLockedRMW() const { return _flags.isSet(LOCKED_RMW); } >>- bool isAcquire() const { return _flags.isSet(ACQUIRE); } >>- bool isRelease() const { return _flags.isSet(RELEASE); } >>- bool isAcquireRelease() const { >>- return _flags.isSet(RELEASE | ACQUIRE); >>- } >> bool isSwap() const { return _flags.isSet(MEM_SWAP|MEM_SWAP_COND); } >> bool isCondSwap() const { return _flags.isSet(MEM_SWAP_COND); } >> bool isMmappedIpr() const { return _flags.isSet(MMAPPED_IPR); } >> bool isClearLL() const { return _flags.isSet(CLEAR_LL); } >> bool isSecure() const { return _flags.isSet(SECURE); } >> bool isPTWalk() const { return _flags.isSet(PT_WALK); } >>- void setAcquire() { _flags.set(ACQUIRE); } >>- void setRelease() { _flags.set(RELEASE); } >> }; >> >> #endif // __MEM_REQUEST_HH__ >>_______________________________________________ >>gem5-dev mailing list >>[email protected] >>http://m5sim.org/mailman/listinfo/gem5-dev >>_______________________________________________ >>gem5-dev mailing list >>[email protected] >>http://m5sim.org/mailman/listinfo/gem5-dev > > >-- IMPORTANT NOTICE: The contents of this email and any attachments are >confidential and may also be privileged. If you are not the intended >recipient, please notify the sender immediately and do not disclose the >contents to any other person, use it for any purpose, or store or copy >the information in any medium. Thank you. > >ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, >Registered in England & Wales, Company No: 2557590 >ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, >Registered in England & Wales, Company No: 2548782 >_______________________________________________ >gem5-dev mailing list >[email protected] >http://m5sim.org/mailman/listinfo/gem5-dev >_______________________________________________ >gem5-dev mailing list >[email protected] >http://m5sim.org/mailman/listinfo/gem5-dev -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782 _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
