@Nilay: Can you follow-up on where you stand on this patch and its derivatives? Do you still think these changes should go in, perhaps in a different form?
Overall, it's really hard to provide review for this, since it introduces substantial conceptual/philosophical changes to SLICC. Thanks! Joel On Thu, Jul 30, 2015 at 10:22 AM, Brad Beckmann <[email protected]> wrote: > > > > 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 > -- Joel Hestness PhD Candidate, Computer Architecture Dept. of Computer Science, University of Wisconsin - Madison http://pages.cs.wisc.edu/~hestness/ _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
