I like the direction of this patch, but I think the buffer_size value should be parsed differently. See below.

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

It appears that this parses the buffer_size value string for a literal, variable name, or a statement containing other variables. Correct? Since this is just parsing that the parser already does, it should probably be handled instead by extending src/mem/slicc/parser.py to allow parsing the value of key-value pairs. That would allow the code to be auto-generated here (or elsewhere) instead of using custom buffer_size-specific code.

Show all issues

1. Brad Beckmann May 14, 2015, 9:32 a.m. (May 14, 2015, 9:32 a.m.)

   I think I understand what your are asking, but I don't believe that is how pair expressions are parsed.  The parser (parser.py) just identifies and parses the pair expression generically.  Whether it is a string or integer, the PairAST object is the same.  It is only in the generation of the code do we understand that this is a MessageBuffer and could have a size associated with it.
   Do I understand correctly?  Or are you asking that MessageBuffers have their own AST?

Ship It!

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

I see. On my first pass here, I misread that this code allows arbitrary expressions for setting buffer_size. You're right that the parser disallows that.
However, you should be using the pair values' types here to be consistent with other code generation rather than using isinstance, which is very out-of-place here. Each PairAST value has a type. From the parser:
def p_pair__assign(self, p):
    """pair : ident '=' STRING
            | ident '=' ident
            | ident '=' NUMBER"""
    p[0] = ast.PairAST(self, p[1], p[3])

And for instance, the parser sets STRING types as follows:
def t_STRING1(self, t):
    r'\"[^"\n]*\"'
    t.type = 'STRING'
    t.value = t.value[1:-1]
    return t

Thus, you should be able to get the buffer_size value's type with code like:
size_type = var["buffer_size"].type

Then you can predicate code generation on whether the type is IDENT, STRING, or INT like other code here (e.g. see 'vtype' local var).
This same issue applies in review request 2814 ( http://reviews.gem5.org/r/2814/ ), which looks like it is just replicating this code?

Show all issues

1. Brad Beckmann May 19, 2015, 10:50 a.m. (May 19, 2015, 10:50 a.m.)

   What you describe sounds like it may work as well, but I don't see why it is any better than the current implementation.  Both provide the same feature.  It doesn't seem worth the effor to reimplement this just because you feel the use of isinstance is out-of-place.

2. Mark Wilkening May 20, 2015, 9:42 a.m. (May 20, 2015, 9:42 a.m.)

   This code seems to follow the conventions used for other properties of message buffers (network, virtual network id, ...). I do not think we need to change this unless we want to completely change how message buffers are handled and make an AST (which I would not reccommend).

3. Joel Hestness May 20, 2015, 10:40 a.m. (May 20, 2015, 10:40 a.m.)

   It's not just that isinstance is out-of-place, it's actually incorrect: You are currently comparing the types of the Python objects that hold the SLICC AST, but you SHOULD be comparing the types defined in the AST itself. These are not necessarily equivalent, nor should we assume that they are.