What do you mean it changes the meaning of ''?  That symbol is not currently used in SLICC.  SLICC has been designed not to directly expose pointers to the programmer, so the '' symbol is currently unused.
Upgrading the transistion construct requires a pretty significant change to the parser.  The current patch is much cleaner and the notation is simplier.

Hi Nilay,

The thought behind the use of '*' was that the nextState could be anystate returned by the getNextState() function....It simply means we do not know the state in advance... Please let us know if you have any other symbol in mind which can better represent this behaviour....

On 5/19 Nilay wrote via email "I understand the thinking behind using '*'.  I don't understand why do we want to fix the associated function's name and its set of parameters.  I think we should be more general over here.  I would against suggest that we allow 'expr' instead of 'ident' as next_state token.  In TransitionDeclAST.py, we should check that the type of the next_state is State, which we always assume exists."
(I believe you meant to say "again" not "against", correct?)
Current the state functions (getState, setState, etc.) are hard coded to particular function names.  What you are suggesting seems like it could be helpful to a very experienced SLICC developer, but changing the harde coded names go beyond this patch.

The patch description shows how the feature is used.  The burden cannot be to provide a public example for every new feature introduced in SLICC.  That hasn't been the bar in the past and it will cause us to keep a lot of useful utility patches internal and our patch queues will continue to grow.

Hi Jason,

As mentioned in the comments, this patch allows a machine to collapse states that differ only in the next state

transition. An example getNextState() function is given below:
State getNextState(Address address) {

    TBE tbe := TBEs[address];

    assert(is_valid(tbe));

    return tbe.NextState;

}
I will try to give a simplified example so that the reviewers will be able to understand the utility of this patch.
Suppose the machine is in state:INVALID_XXX. Also assume that the machine can move to state:EXCLUSIVE or state:SHARED depending on the response it gets from the directory/lower-level caches. However, before it moves to any of these final state, assume that it has to wait for acknowledgements from its peer caches. So, without the suppport of this patch, the slicc code will look as follows:
transition(INVALID_XXX, ResponseX, INVALID_EXCLUSIVE){

  do_something;

}
transition(INVALID_XXX, ResponseS, INVALID_SHARED){

  do_something;

}
transition(INVALID_EXCLUSIVE, ALL_ACKS, EXCLUSIVE){

  do_something;

}
transition(INVALID_SHARED, ALL_ACKS, EXCLUSIVE){

  do_something;

}

Now, with the support of this patch, the above code can be re-written in fewer lines as follows:
transition(INVALID_XXX, {ResponseX, ResponseS} , INVALID_WAITFORACKS){

  set_nextState;

}
transition(INVALID_WAITFORACKS, ALL_ACKS, *){

  do_something;

}
Note the reduction in the number of transitions. Now, this patch can save tones of time and reduce number of lines of code substantially while writing a complex protocol. Hope this helps!!!

Ah, thanks for the thorough explanation. Do you think you could add this to the wiki?

Should I update it here : http://gem5.org/SLICC ??

Um... nothing changed in the updated patch?
I still dislike binding implicit meaning to '*'.

With the "wildcard next state" approach, the programmer do not know the next state while writing the protocol. So, the programmer has to write a transition with the only information he has at that time which is the next state can be "any state". From a programming perspective, I think '' is the most appropriate character to capture/represent this sentiment. Even more so becasue '' is widely used in programming languages as the wildcard character.
Now, the question "which is the next state?" is resolved by the getNextState() function during runtime. Please see my detailed response to Jason's comment to understand how exactly this functionality can be used.

Joel, please let me know if the above explanation makes sense. If no more concerns, please give it a ship it!

I understand what this patch does, but I'm not a fan of binding '*' to a specific function. Changes to the SLICC language should be extensible, and this is not. If this wildcard-next-state operator gets into mainline code, it will be a fair amount of work to refactor it and make it extensible.
Also, you (Brad) asked for more comments on the 'updated' patch, but the patch wasn't actually updated on Reviewboard. Did something actually change?

Regarding the 'updated' patch, actually the patch did not change. Apparently, a lot of patches from "Tony Gutierrez" got reposted without any change to those patches. Probably, some scripting issue.
Coming back to the comments on the patch, just to clear my confusion, are you proposing '*' symbol should be replaced with some other symbol or are you proposing that wildcard-next-state functionality should be implemented in some other way? And how does this change makes SLICC nonextensible and difficult to refactor?

As I stated on the email thread: "Using '*' to designate the specific function 'getNextState()' binds implicit semantics of a getNextState function when it is present. In Sooraj's example, you'd only be able to get the next state from the TBE's nextState variable (unless you have a way to pass parameters to getNextState() somehow?). If the third transition parameter were parsed as an expr, you could put in different [added: next state] functions for different sets of "wildcard" transitions, and you'd avoid introducing any implicit function declaration semantics."
In other words, this is not extensible, because binding the meaning of '*' to a specific function disallows other functions from being used to get the next state. It seems like such a situation would arise pretty quickly as soon as one starts grouping wildcard transitions. Further, this patch would need to be effectively reverted to add multiple next state functions, and we'd have to remove or deprecate uses of '*' as a wildcard operator.
Nilay described what it would take to call next state functions in transition headers in lieu of this patch (also on the email thread):

"There are several things need to be tackled here:

a. we specify states and events in transitions as 'ident' and not as 'enum'.  This is different from how we use them in rest of the code.

b. variables specified in the next state function call would need to be added to the symbol table.  This means that we would have to fix the set of parameters anyway."
As it stands, I oppose this patch. Nice-to-have features - like wildcard next states on transitions, here - should not introduce new SLICC language constructs unless the feature cannot be implemented any other way.

Thanks for the reply Joel.

I think this patch has to be looked upon as an extension of the exisiting SLICC philosophy.

1) Reserved functions: SLICC already has reserved functions like "getState()" and "setState()" which are used to get and set states of "this" transition. The getNextState function introduced by this patch is similar to those functions except that its functionality is different. So, I think there is no conflict with SLICC philosophy when we add one more reserved function to SLICC.

2) Third transition parameter as an expr: Well, one can add any logic (or expression) in the getNextState() function in the same way one can add any logic inside the existing getState function. There is absolutely nothing which the getNextState function is doing different from the existing getState fuction of SLICC in this regard, other than providing an interface function. What is inside that interface function is completely under the control of protocol developer. Now, regarding the tbe example which I showed, that was just one example on how to use this new functionality. Additionally, one can modify the set_NextState action (see my response to Jason) to have any expression one needs so that the next state is computed with the help of any complex expression of protocol developer's choice.

3) '' symbol is eternally bound to getNextState function: In the context of this patch, '' is the symbol used to distinguish normal trnasitions from the wildcard_next_state transitions that require getNextState function. Currently, we do not have any special meaning associated with '*' in SLICC; so that is not conflicting with SLICC either. Now, if you have some other symbol in mind which better reflects this wildcard feature, please let me know.

4) parameter list of getNextState: Currently, the getNextState() is implemented to just return the next state from tbe entry or any other global structure defined in the protocol file. This is a clever way of avoiding the need to pass unknown number of parameters which might be needed in the future. If someone wants to make a complex next state expresion, they can do it in some action (like setNextState action in my example to Jason) and place the result of this computation in the tbe. This is very much similar to getState function which only looks into cache_entry or tbe_entry for finding the next state.
Now, this patch is not introducing just a new feature, but an incredibly powerful feature for protocol writers saving them from the state explosion problem of complex protocols and tones of time in the process. So, given the merits of this patch, I think this patch should not be discarded. If some developer finds the need of improvement for this patch in the future, they can always improve on this patch as this patch is not breaking any of the fundamental architetcure of SLICC.

I appreciate your effort on this. Apologies if the root of my opposition wasn't clear. Allow me to clarify:





"I think this patch has to be looked upon as an extension of the exisiting SLICC philosophy."


My opposition is that this patch is trying to extend a SLICC philosophy/vision without following a couple SLICC conventions. First, let's be clear that you're referring to the major SLICC philosophy/vision that is: to provide a compact domain-specific language with which to define state machines. I completely agree that collapsing transition declarations is very important for achieving compact code. I also agree that it would be great to introduce a way to collapse transitions which have the same input state and event, but different output states (i.e. the specific aim of this SLICC change).
I oppose introducing the '*' symbol and binding implicit meaning to it to achieve this transition compaction. Using '*' does not follow two SLICC conventions:
a) SLICC aims to avoid syntactic sugar. This patch adds a new and unnecessary symbol to the language. It is unnecessary, because the same desired functionality can be achieved simply by changing the way we parse transition headers.
b) SLICC aims for transition handling be completely explicit in transition headers, which is important for compact code clarity. Current transition headers explicitly declare the current state, trigger event, and next state. Allowing variable output states will necessarily make it difficult to maintain this convention. However, using the wildcard operator as introduced by this patch really obfuscates the output state, requiring a user to know that '*' means they need to inspect getNextState() to see how the next state is set. At least if the output state were parsed as an expr, there would be an explicit call to the next state function, so a user would not need to know the implicit binding from '*' to getNextState().
These convention problems are in addition to the binding only allowing a single getNextState function and the longer-term problems that stripping out the '*' operator later will be painful.
Given that this is an advanced feature, I don't feel that the merits of this patch outweigh the slacked SLICC conventions that it imposes. I feel strongly that this should be implemented without adding the '*' symbol to the language.

Just a quick note:





"I think there is no conflict..."

"that is not conflicting with..."


Arguments such as these can't really be used to support a patch. Patches should also avoid breaking compilation, for instance, but this is a vacuous requirement. Patches should do something to improve the codebase rather than not do something that harms it.
Statements like this can be used to refute arguments that claim conflicts, but I've neither claimed that getNextState() conflicts with SLICC reserved functions nor that introducing the '*' symbol conflicts with other SLICC syntax.

Hi Joel,

We’d really like to check this in despite your objection, and finish dealing with the issue later once the project management committee is in place to handle disagreements like this.

It does seem that we would be unable to move this forward without further effort or input from other community members. While I'd rather not see this patch go in, you can push it, so we'll be able to see further new code from AMD, and I can advocate for/help with adjusting this implementation at a later date.
Thank you for your input on limitations and aims of SLICC.