I need a clarification on the proposed syntax. Is one port being nested inside the other?  For example

in_port() {
  in_port() {
  }
}

Or the two in_ports have separate declarations (not nested), but they share the same message buffer?

src/mem/slicc/ast/PeekStatementAST.py (Diff revision 1)

Could you explain why an exception is required here? Looking through the rest of the gem5 code, the only other times we use exceptions are in the python-c++ bindings and our string wrappers.
If using the exception is really required (or if it is significantly easier to implement with exceptions) I don't have any problems with it, but it seems like there is almost always a more elegant solution. I think it's worth a little explanation since we so rarely use them in gem5.

Show all issues

1. Brad Beckmann May 13, 2015, 11:32 a.m. (May 13, 2015, 11:32 a.m.)

   I think exceptions are needed because SLICC creates the AST in a single pass.  I certainly don't know how to do this in another way.  Derek Hower could provide you a more detailed response.

2. Nilay Vaish May 14, 2015, 7:09 a.m. (May 14, 2015, 7:09 a.m.)

   I am also against using exceptions for doing actual work.  Exceptions are for error handling, not for writing code.
   I think the following would work.  The SLICC syntax would be:
   
   in_port(ResponseQueue_in, {ResponseMsg, TgtResponseMsg}, responseFromDir, rank=3)
   
   The order of the message types would decide the order in which each type is tried.
   
   The generated code would look like:
   // Declare message
   const $mtid* in_msg_ptr M5_VAR_USED;
   if (check for first type) {
      do the required work
   }  else if (check for next type) {
   } else {
     fatal / panic.
   }

3. Brad Beckmann May 18, 2015, 5:05 p.m. (May 18, 2015, 5:05 p.m.)

   Your suggestion results in more complicated nested code.  One of the benefits of this patch is it allows the SLICC developer to write the in_port logic as two separate sets of conditional logic.

4. Jason Lowe-Power May 28, 2015, 5:37 p.m. (May 28, 2015, 5:37 p.m.)

   I don't think that this has been addressed. I really don't like that this is one of the only places we use C++ exceptions in gem5. I would be quite surprised if this is truely the only way to implement this code. Any chance you could reach out to Derek on this to see if he remembers why he did it this way?
   However, since I don't have the initiative to suggest a better solution, I won't block the patch based on this. It's more of a style thing anyway.

src/mem/slicc/symbols/StateMachine.py (Diff revision 1)

I think this should be panic, not fatal. It's on the border so I could easily be persuaded. But I think it would be useful to have an abort() here, not an exit(1) call. Note: 8.1 Fatal v. Panic. It's mostly that this error happens during runtime, not at configuration time, that makes me think it should be a panic. (It's also a panic below in the other case of a failure.)

Show all issues

1. Brad Beckmann May 13, 2015, 11:32 a.m. (May 13, 2015, 11:32 a.m.)

   I completely understand your point, but invalid protocol transitions also invoke a fatal.
   If I change this error to panic, we should also change the invalid transistion error.  Do you agree with making them both panics?

2. Brad Beckmann May 18, 2015, 5:05 p.m. (May 18, 2015, 5:05 p.m.)

   Ping???  I'm interested to get people's thoughts here.

3. Jason Lowe-Power May 18, 2015, 5:15 p.m. (May 18, 2015, 5:15 p.m.)

   Sorry for the slow response. Yes, I agree with making them both panic.

4. Brad Beckmann May 28, 2015, 5:33 p.m. (May 28, 2015, 5:33 p.m.)

   Any other comments on this patch?  Is everyone else ok that we make all these fatals panics instead?