> 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.
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. - Nilay ----------------------------------------------------------- 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
