> On July 24, 2015, 3:13 p.m., Brad Beckmann wrote: > > I'm surprised you moved forward on this patch. I thought I had > > communicated over email a few weeks ago that we should not go down this > > path before a lot of careful thought and debate on whether this is a good > > thing to do. Personally I think this patch removes many of the benefits of > > having a SLICC ast and goes against many of the main design principles of > > SLICC. SLICC is designed to avoid having the programmer worry about memory > > management. SLICC is a layered development approach where the sm file is > > only meant to considering the protocol logic. I believe that is why SLICC > > has been so successful over the past 16+ years. It allows one to create > > differnet protocols without requiring one to write C++ and handle > > complicated memory management. This patch removes those benefits. > > > > Please do not check this in. We need to come to a consensus on whether > > this is even a good direction to go in first. > > Brad Beckmann wrote: > On 7/26 Nilay wrote "ruby uses shared pointers for Message objects. But > messages are not shared by multiple objects. At a given time, only one > object holds the pointer to a message object. So there should not be any > need for messages to be shared pointers. They can be deleted as soon as the > current owner realizes that the message is no longer required. In my > opinion, the question memory management does not arise at all." > > The next patch you introduce (2989) builds on this patch and introduces > new calls to each action that creates a message. That sure seems like to me > that these series of patches are bringing up the issues and questions of > memory management to the sm programmer. > > > On 7/26 Nilay wrote "I don't know how to measure the success of a > language, more so when it is allowed to undergo changes. Comparing the ISA > description language and SLICC, I find that SLICC has was change 33% times > more even though it has been around for only half the time. To me this means > that SLICC is not as stable as it can be. And I think the major reason is > that it attempts to do things that are better done in plain C++." > > SLICC was created by Milo back in 1999. I'm not positive, but the ISA > description generator was created by Steve and/or Nate in the early 2000's. > I believe SLICC predates the ISA generator. Steve has noted to me that the > main reason the ISA generator did not create a full-blown AST like SLICC, is > simply because they didn't want to put in all that work. If work was not an > issue, I think that the ISA parser could benefit from a full AST like SLICC. > Milo put in a lot of work 16 years ago and we've been building and benefiting > from that ever since. There is more to SLICC from a language prespective and > new research protocols evolve faster than ISAs. I think that is why you see > more change in SLICC than the ISA generator. > > Instead of exposing C++ memory management to sm programmers, how about we > consider new and improve language improvements to handle message creation. > Perhaps we can introduce a new enqueue function? > > > On 7/26 Nilay wrote "shared_pointers have a cost. I think the savings > made is substantial enough to justify the required changes to current > protocols. My back of the envelope calculations tell me that for a research > group like AMD Research, it makes economic sense as well." > > The SLICC language does not require shared pointers and for many years > SLICC simply generated raw C++ pointers. We should be able to elimiate > shared pointers without impacting the protocol sm files. Also what back of > the envelope calculations make you think this change makes economic sense to > AMD? The economics of incorporating this patch and 2889 is one of the main > reasons I'm against it. Rewriting and verifying all actions in our internal > and soon to be public protocols would require $10k's of work. > > Nilay Vaish wrote: > First of all, you are very diligent about using the reviewboard. One of > the problems I face with the reviewboard is that I cannot reply inline. > From now on, I am going to do what you did: copy the response of the > previous poster. But I really wish that reviewboard automatically > incorporates > email responses. > > > On Tue, 28 Jul 2015, Brad Beckmann wrote: > > > > > > > > > On 7/26 Nilay wrote "ruby uses shared pointers for Message objects. > > But messages are not shared by multiple objects. At a given time, only > > one object holds the pointer to a message object. So there should not > > be any need for messages to be shared pointers. They can be deleted as > > soon as the current owner realizes that the message is no longer > > required. In my opinion, the question memory management does not arise > > at all." > > > > The next patch you introduce (2989) builds on this patch and introduces > > new calls to each action that creates a message. That sure seems like > > to me that these series of patches are bringing up the issues and > > questions of memory management to the sm programmer. > > Brad, what I meant was that you do not have to do anything special. We > allocate > and release memory all the time. There are places in ruby where we > allocate and > release memory. In a protocol's case, the component requiring a new > message would > allocate and the component receiving it would deallocate it. Again, > nothing special > or out of ordinary. > > > > > > > On 7/26 Nilay wrote "I don't know how to measure the success of a > > language, more so when it is allowed to undergo changes. Comparing the > > ISA description language and SLICC, I find that SLICC has was change 33% > > times more even though it has been around for only half the time. To me > > this means that SLICC is not as stable as it can be. And I think the > > major reason is that it attempts to do things that are better done in > > plain C++." > > > > SLICC was created by Milo back in 1999. I'm not positive, but the ISA > > description generator was created by Steve and/or Nate in the early > > 2000's. I believe SLICC predates the ISA generator. Steve has noted to > > me that the main reason the ISA generator did not create a full-blown > > AST like SLICC, is simply because they didn't want to put in all that > > work. If work was not an issue, I think that the ISA parser could > > benefit from a full AST like SLICC. Milo put in a lot of work 16 years > > ago and we've been building and benefiting from that ever since. There > > is more to SLICC from a language prespective and new research protocols > > evolve faster than ISAs. I think that is why you see more change in > > SLICC than the ISA generator. > > > > Instead of exposing C++ memory management to sm programmers, how about > > we consider new and improve language improvements to handle message > > creation. Perhaps we can introduce a new enqueue function? > > > > I am not in favour of this option for the time being. > > > > > On 7/26 Nilay wrote "shared_pointers have a cost. I think the savings > > made is substantial enough to justify the required changes to current > > protocols. My back of the envelope calculations tell me that for a > > research group like AMD Research, it makes economic sense as well." > > > > The SLICC language does not require shared pointers and for many years > > SLICC simply generated raw C++ pointers. We should be able to elimiate > > shared pointers without impacting the protocol sm files. Also what back > > of the envelope calculations make you think this change makes economic > > sense to AMD? The economics of incorporating this patch and 2889 is one > > of the main reasons I'm against it. Rewriting and verifying all actions > > in our internal and soon to be public protocols would require $10k's of > > work. > > > > > > This is the point that I wanted to come to. The question here is how are > the changes going to be made. I have setup things in a way that you > would only need to delete the current action code and paste some code > that > would appear in the generated controller. > > For example, assume you have a .sm file with some actions defined in it. > The patch before this patch would generate a controller file that would > contain the SLICC code with actions that have c++ code blocks. You would > need to cut the current action code in your .sm file and paste the code > that > appears in the generated controller file. It would be exactly one single > delete to the original .sm file and one single copy from the generated > file. > I think it would not take more than a minute to modify one single .sm file > once SLICC compiler has processed the protocol. > > And by eliminating shared pointers, we would save simulation time that > should > translate into savings on compute resources. I think, at AMD Research, > 5% savings > on compute resources would be substantial enough to justify this effort > required > to change the protocols. I did some calculations using cost of computing > resources for Google Cloud (about 20 cents / hr) and assuming 1,000,000 > hours of > simulation time a year. I think 5% translates to $10K. > > Brad Beckmann wrote: > Can you explain why you are not in favor of changing the implementation > of the enqueue function? Why do you think we have to introduce c++ into the > actions in order to eliminate shared pointers? The SLICC language does not > require the shared pointer implementation. > > Nilay Vaish wrote: > We can change the implementation of the enqueue() function. But then we > will > have to change either the dequeue() function to delete messages (implicit > deletion), > or add delete functionality to SLICC (explicit deletion). I do not want > to go with > implicit deletion since ultimately I would like to control when the > messages are > deleted. Having explicit deletions allow us to remove RubyRequest, > another structure > that is not required and is only an overhead. > > I did consider adding delete functionality to SLICC, but I do not like > it because we are essentially adding more stuff from C++ to SLICC. I > would rather > have the whole of C++ available and let the user do whatever they want. > > Brad Beckmann wrote: > Well I think we have reached the crux of the disagreement here. You want > to control when messages are deleted in the sm files. My contention is that > explicitly deleting messages in the sm files goes against one of the > fundamental tenets of SLICC. I'm not quite sure how we resolve our > disagreement, but hopefully we can agree that is the real disagreement. The > performance overhead of shared pointers is orthogonal. I'm ok with removing > shared pointers. I just want to maintain the implicit and consistent > deletion of messages. > > Nilay Vaish wrote: > So if I agree to provide both implicit and explicit deletion, would you > agree on having c++ code blocks in actions?
I'm ok with allowing c++ code in the action blocks, but I'm against requiring c++ code in the action blocks. I believe we need to maintain backward compatibility to all existing action definitions. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2988/#review6821 ----------------------------------------------------------- On July 24, 2015, 4:45 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2988/ > ----------------------------------------------------------- > > (Updated July 24, 2015, 4:45 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10951:6cc98e435866 > --------------------------- > ruby: slicc: have c++ code in action blocks > > This patch changes the way code in action blocks is specified. Right now > we write SLICC code in action blocks, this patch proposes to have c++ code > instead. This is required for moving away from reference counted messages > currently in use by the ruby memory system. Also, from a longer term > perspective, this is required for writing a coherence protocol for the classic > memory system since most of the code for the protocol would be sourced > from the current c++ code in the classic memory system. > > > Diffs > ----- > > src/mem/protocol/MESI_Two_Level-L1cache.sm 9689ead7b479 > src/mem/protocol/MESI_Two_Level-L2cache.sm 9689ead7b479 > src/mem/protocol/MESI_Two_Level-dir.sm 9689ead7b479 > src/mem/protocol/MESI_Two_Level-dma.sm 9689ead7b479 > src/mem/slicc/ast/ActionDeclAST.py 9689ead7b479 > src/mem/slicc/parser.py 9689ead7b479 > src/mem/slicc/symbols/Action.py 9689ead7b479 > src/mem/slicc/symbols/StateMachine.py 9689ead7b479 > src/mem/slicc/symbols/Transition.py 9689ead7b479 > > Diff: http://reviews.gem5.org/r/2988/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
