src/cpu/testers/rubytest/CheckTable.cc (Diff revision 1)

These adjacent DPRINTFs should be consolidated into one.  It's more efficient (since it's a single check of the trace flag and a single call to cprintf), and in this case (and many others) we don't really need the function name, file name, and line number printed redundantly.

Also you don't need to use c_str(), cprintf() handles std::string just fine (and anything else that supports the '<<' operator, I believe).

1. Nilay Vaish Oct. 23, 2010, 6:52 a.m. (Oct. 23, 2010, 6:52 a.m.)

   Does that mean that format specifiers used in DPRINTF() statements are just place holders? In not, what would be the format specifier for printing an object of some class that supports << operator?

src/mem/SConscript (Diff revision 1)

Do we want to define a whole new disjoint set of trace flags just for Ruby?  For example, why have RubyCache and Cache both?

1. Brad Beckmann Oct. 26, 2010, 2:33 a.m. (Oct. 26, 2010, 2:33 a.m.)

   I don't think we need a whole disjoint set of trace flags and for most new flags, I don't think we need to explicitly identify them as Ruby flags.  However, I can see how one would want a separate RubyCache flag since we currently have both a RubyCache and Cache configuration python object.  Overall, I'm indifferent and I'm willing to let Nilay use his best judgement here or let others insist on one way or another.

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 1)

This DPRINTF is broken (and several others like it)... it needs a trace flag as the first arg, and "%str" probably isn't the format string you want (basically it's just like printf, so you probably mean "%s").

1. Nilay Vaish Oct. 21, 2010, 9:40 p.m. (Oct. 21, 2010, 9:40 p.m.)

   The DPRINTF statements that appear in .sm files are re-written by the SLICC compiler. SLICC compiler provides the trace flag. Looking at the format specifier, the compiler will add a .c_str() after print_str(). But in a comment above, you mentioned that c_str() is not required. May be we can drop %str altogether. I need to look in to this.

2. Steve Reinhardt Oct. 21, 2010, 11:56 p.m. (Oct. 21, 2010, 11:56 p.m.)

   OK, thanks for the clarification.  I missed the SLICC compiler code before but I found it now.  I have two comments on that:
   - Unless there's a real need to have a different syntax, it would be nice to just write the SLICC code as a regular DPRINTF and pass DPRINTF calls through unmodified.  I'd like to better understand why you think it's needed (if you still think it's needed after making the c_str() change).
   - If we do end up needing a DPRINTF replacement with different syntax in SLICC, let's call it something other than DPRINTF to make it clear that it is different.
   

3. Nilay Vaish Oct. 23, 2010, 6:59 a.m. (Oct. 23, 2010, 6:59 a.m.)

   DPRINTF() calls will have to be modified since we want to add line number of the .sm files where the call appears. I think we can maintain the syntax of DPRINTF() as used in other places. But in order to combine adjacent DPRINTF() statements, we would need change to the way DPRINTF() call is processed. As per my understanding, currently SLICC compiler assumes that only one object / variable would be printed at a time.

4. Brad Beckmann Oct. 26, 2010, 2:26 a.m. (Oct. 26, 2010, 2:26 a.m.)

   I would prefer not to have the .sm DPRINTF calls look exactly like the c++ calls.  In particular, it seems unecessary to specify the trace flag "RubySlicc" at every DPRINTF statement.  I don't think we'll ever need more than one trace flag for the .sm debug statements.  We've never needed more than one in the past.  Also by removing the trace flag, it will be one less thing (though I understand it is relatively minor) for the protocol programmer to worry about.

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 1)

We should get rid of these commented-out lines, either deleting them if they're not needed or just using a trace flag that keeps them turned off unless wanted.  That's the point of the trace flags, is so that you don't have to comment out debug printfs, but can just turn them on and off dynamically at the granularity you want.

1. Nilay Vaish Oct. 21, 2010, 9:41 p.m. (Oct. 21, 2010, 9:41 p.m.)

   I can remove the commented lines, but I may not be the right person to make that decision.

2. Steve Reinhardt Oct. 22, 2010, 12:10 a.m. (Oct. 22, 2010, 12:10 a.m.)

   I would lean toward uncommenting them rather than removing them, and then if people later find them too verbose they can tweak the trace flags to turn them on more selectively.  Note that in some cases we have a Foo flag and a FooVerbose flag to get different levels of tracing on the same category of DPRINTFs... not that we need to do that here, but it's an option.  In the cases where the commented-out lines are adjacent to uncommented lines, I expect they could be condensed into a single DPRINTF.
   

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 1)

Note that DPRINTF automatically prepends the value of name(), so we should make sure that works for Ruby so we don't need to explicitly pass m_name.c_str() (which I assume is the same or similar).

src/mem/ruby/common/Address.hh (Diff revision 1)

Indentation looks funky on this line.  Are you using tabs?

src/mem/ruby/common/Address.hh (Diff revision 1)

Nate can correct me if I'm wrong, but the cprintf library eventually uses '<<' at the bottom, so if we need to define a type-specific output (here and with DataBlock below) we should just overload that operator.  In this specific case, much of this formatting can be handled in the format string itself as "[0x%x, line 0x%x]" and then calling with addr and blockAlign(addr) as arguments.

src/mem/ruby/common/Debug.hh (Diff revision 1)

We need to decide how valuable it is to automatically include the function name, file, and line number.  If there's a consensus that we should keep it, we should add DPRINTF variants to M5 that have this.  If not, we should eliminate this info from the Ruby debug messages.  But we shouldn't take a bunch of boilerplate that used to be nicely encapsulated in a macro and reproduce it N times all over the code.

src/mem/ruby/common/Debug.cc (Diff revision 1)

Code should never just be commented out... if we don't need it any more, just delete it.

I think Steve covered most of the main points.  Please see my detailed comment below about when I think line numbers and function names are useful.

Brad

src/mem/protocol/RubySlicc_MemControl.sm (Diff revision 1)

I'll beat Nate to this.  :)  Try to avoid inserting and deleting random lines in files.  It cluters the diff and unecessarily makes merging harder.

src/mem/ruby/common/DataBlock.hh (Diff revision 1)

Spacing is off.  Are you using mistakingly using tabs?

src/mem/ruby/common/Debug.hh (Diff revision 1)

I agree with Steve's point that adding "__PRETTY_FUNCTION__, __FILE__, __LINE__" to every Ruby DPRINTF call is too cumbersome and we either need to remove it or define a macro that does it automatically.  Nilay, I think I might have confused you during our previous email conversations.  We definitely want to automatically include the function name, file, and line number for the SLICC/.sm debug statements...I've found that feature very useful in the past.  However, I'm not sure we need it for the debug statements on the Ruby side.  Nilay, if I understand your changes to SLICC correctly and recall from our previous conversations on this topic, these old ruby DEBUG macros relied on the ostream operator<< overloading, correct? That is why you now have the conditional statements in the SLICC that generate the correct DPRINTF line depending on the type.  Am I understanding the issues correctly?  If so, I think your solution for the SLICC generation of DPRINTF statements looks good.  I just think that we need to clean up the Ruby DPRINTF statements.

1. Nilay Vaish Oct. 21, 2010, 9:37 p.m. (Oct. 21, 2010, 9:37 p.m.)

   I will remove the function name, file name, line number from Ruby DPRINTF statements.

src/mem/ruby/common/NetDest.hh (Diff revision 1)

Spacing again.

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

See long comment above.

Overall, I have two major comments.  1) You should get rid of print_str() (and probably print()) for that matter and implement operator<<.  2) You shouldn't be duplicating so many lines with the __PRETTY_FUNCTION__, __FILE__, __LINE__ stuff.  I think it should be buried in a macro.  In fact, if people like, we could make those available to *ALL* DPRINTFs and make turning them on and off another trace flag.  In all honesty though, they tend not to be necessary if your trace strings are at all descriptive.

src/cpu/testers/rubytest/CheckTable.cc (Diff revision 1)

You don't need __PRETTY_FUNCTION__ and __FILE__/__LINE__.  I'd say that one or the other should do.  Also if you're going to do this a lot, we should have a new DPRINTF macro for it.

src/mem/SConscript (Diff revision 1)

I would emphatically argue *no*

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 1)

I agree that we should pass it through unmodified.

Also, DPRINTF understands operator<<, and if you use %s, it can use any object with an operator<<, so I'd get rid of print_str() on the objects and just rename it operator<<

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 1)

I agree with Steve.  Commented out code is a no-no.

src/mem/protocol/MESI_CMP_directory-L2cache.sm (Diff revision 1)

Can you consolidate these into a single expression that has some context?  I guess that's not the point of this diff though, so I don't think it's required.

src/mem/protocol/MESI_CMP_directory-L2cache.sm (Diff revision 1)

Same here

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 1)

We should probably change this to operator<<, then again, I am not quite clear how ruby uses this.

src/mem/ruby/SConsopts (Diff revision 1)

Good work!

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 1)

I'd rather see a new DPRINT Macro.

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 1)

Generally, this is a bad idea.

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 1)

you don't need to use c_str(), and in fact name() is automatically inserted into all DPRINTF statements.

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 1)

why is "message" in the argument list?  Also, why this very complicated expression to get some message string?  I suggest getting rid of print_str() and making operator<< work.

src/mem/ruby/common/Address.hh (Diff revision 1)

I'd honestly get rid of both print and print_str and just simply implement operator<<

src/mem/ruby/common/Address.hh (Diff revision 1)

Yes.  That is exactly correct.

src/mem/ruby/network/simple/PerfectSwitch.cc (Diff revision 1)

Pretty strange that you keep having things like "m_switch_id" in the argument list.

src/mem/ruby/storebuffer/storebuffer.cc (Diff revision 1)

Instead of commenting these out, you could add a new Trace flag for these and get rid of the #ifdefs.  Not sure if it's useful or not though.

Thanks, this is hugely improved.  Just a few little things I noticed still; the panic() thing is the only one I consider really necessary to take care of before committing.

src/mem/SConscript (Diff revision 2)

Nate and I agreed that it would be nice not to prefix all of these with "Ruby", though if other people feel this decision needs more discussion that's fine.

1. Nilay Vaish Oct. 25, 2010, 12:56 a.m. (Oct. 25, 2010, 12:56 a.m.)

   To me it seems that cache modeling is forked into two portions, one under the cache directory and another under the ruby directory. Therefore, I would prefer having separate trace flags for these two.

src/mem/protocol/MI_example-dir.sm (Diff revision 2)

Can some of these FooBarBazType_to_string() functions, like the one here and several more below, be converted to (or hidden behind) operator<<?  That's not strictly necessary but it would make these calls a lot less verbose.

1. Nilay Vaish Oct. 24, 2010, 4:22 p.m. (Oct. 24, 2010, 4:22 p.m.)

   I will make these changes and update the diff soon.

src/mem/ruby/storebuffer/storebuffer.cc (Diff revision 2)

In these cases (and the ones below) where this is really a fatal error, I believe the whole ERROR_OUT/DEBUG_EXPR/ASSERT(0) sequence should be replaced by a single call to panic(), which takes the same arguments as cprintf().  I think if you're dying you don't want to only print relevant info if you had traceflags enabled too.

1. Nilay Vaish Oct. 24, 2010, 3:57 p.m. (Oct. 24, 2010, 3:57 p.m.)

   Let's handle ERROR_OUT calls in a different set of changes. We will be going through Ruby's assert mechanism soon.

src/mem/slicc/ast/FuncCallExprAST.py (Diff revision 2)

What is this code doing?  It looks like it's inserting the location, but how does this work if the format string isn't changed?  Plus it would be nice to have a comment here in the SLICC code too.

1. Nilay Vaish Oct. 24, 2010, 4:15 p.m. (Oct. 24, 2010, 4:15 p.m.)

   The code inserts the location of the DPRINTF() statement in the .sm file in the statement it self. The second argument in the call to 'code()' function represents the location. This is placed where $0 appears in the first argument of the 'code()' function. 'format' represents the second argument of the original DPRINTF() call. It is left unmodified. str_list is used for concatenating the argument list following the format specifier in a DPRINTF() call. A DPRINTF() call may or may not contain any arguments following the format specifier. Consider the examples DPRINTF(RubySlicc,"Hello!") and DPRINTF(RubySlicc,"%s",address). These two need to be handled differently. Hence the check whether or not the str_list is empty.

src/mem/protocol/MESI_CMP_directory-L2cache.sm (Diff revision 3)

It looks like this call goes over 80 chars on the first line, and then uses up a lot of unnecessary lines by taking up a whole line for each arg.  If you want to keep the verbose labels, you should format it like this:

DPRINTF(RubySlicc, "Address: %s, L2 Cache State: %s, Sender: %s, "
        "Response Type: %s, Destination: %s",
        in_msg.Address, getState(in_msg.Address), in_msg.Sender,
        in_msg.Type, in_msg.Destination);

though my inclination would be to save more space by abbreviating or even eliminating some of the labels in the format string (since most of the fields will be self-evident, and they'll get pretty tiring in the trace output too, I expect).

Looking much better.
There are two main questions:
1) Do we want Ruby to prefix all trace flag names.  (I vote NO)
2) Do we want to have you insert the location into the DPRINTF statements or use #line directives and create DPRINTFL.  The latter is more general, but it could certainly be done in the future.

There are quite a number of lines that are over 80 columns, can you fix those?

src/mem/SConscript (Diff revision 3)

I still think that you should use more general names.  Try to use the names that the M5 cache hierarchy does.  As we merge the two, it would be better that way.

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 3)

Can you please make sure that you don't create lines with more than 80 columns.

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 3)

Again

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 3)

80 columns

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 3)

80 columns

src/mem/protocol/MESI_CMP_directory-L2cache.sm (Diff revision 3)

80 columns

src/mem/protocol/MESI_CMP_directory-L2cache.sm (Diff revision 3)

I completely agree.

src/mem/ruby/common/NetDest.hh (Diff revision 3)

Does this compile?  Ruby isn't a trace flag.

src/mem/slicc/ast/FuncCallExprAST.py (Diff revision 3)

I think it would be better if we emitted #line statements in the output code instead of doing this.  That would allow us to get better compiler errors too.  That could be done in the future I guess.

Overall, it looks good to me.  The only thing I would like to see changes is removing the RubySlicc flag from the .sm DPRINTF statements.