#pragma GCC system_header can be used to get around the __SUNPRO... problem without modifying the SystemC headers or angering the warn->error flags in gem5.

(Tried in GCC, not in clang (#pragma clang system_header) but I believe it supports the pragma in the same way)

src/sim/eventq.hh (Diff revision 1)

(I'm attaching this comment here as the eventq.cc patch isn't currently visible)

I've tried a small example passing gem5 memory request around using the SystemC patch.  I've hit a problem with issuing gem5 events from SystemC as getCurTick() is allowed to lag sc_time_stamp.

Both EventQueue::insert and EventQueue::remove use setCurTick to correct the time difference but it's quite a common paradigm in gem5 to do:

event.schedule(... getCurTick() + something ...)

Where getCurTick will return the wrong value.

Obviously simple cases can be solved by using the 'right' source of time (and I can imagine scenarios where you might actually want to cultivate a time difference if you were, say, to have some kind of quantum keeping between SystemC and gem5).  Unfortunately, when the getCurTick is deep in code you have no control of (my immediate problem was with a schedule in SimpleMemory called from sendTimingReq from SystemC) it's harder to solve.

I'd suggest a few solutions:

1) Make the two times *always* agree.  Use sc_time_stamp().value() for getCurTick.

2) Provide a 'catchUp' member function that can be called before any potentially curTick-accessing event.

In both cases I'd suggest that the current setCurTicks in the SystemC modified EventQueue use an internal 'catchUp' and that that function should assert that the gem5 time is earlier than the SystemC time and that no events are currently scheduled between the two times.

There is another problem.  EventQueue isn't actually used only for mainEventQueue.

The CPU models use (very ugly in my opinion) EventQueues to sequence events based on instruction counts rather than Tick time.

Look at line 140 in src/cpu/base.cc for a declaration and the function instDone at line 1523 in src/cpu/o3/cpu.cc for examples.

I would suggest, to solve both of these problems, that when using the SystemC extension EventQueue should have a shallow class hierarchy.

Have EventQueue in its current, non SystemC form as the base (with the 'base is most common implementation' philosophy), make either the insert/remove/getCurTick interfaces virtual (or conditionally virtual if the virtual call scares people) or have a virtual 'synchronise/catchUp' that is empty in EventQueue and override those functions in a SystemCEventQueue derived class of which mainEventQueue is the only (rare) instance.

Having that derived class would also give a home to the Gem5EventQueue class/instance and EventQueue::returnEvent.

The event queue notify in Event::insertBefore could be implemented by an empty EventQueue::notifyInserted function overridden in SystemCEventQueue.

Show all issues

1. Steve Reinhardt June 21, 2013, 1:45 p.m. (June 21, 2013, 1:45 p.m.)

   If it's helpful to create a separate EventQueue-like class to handle the instruction-count-based events, I don't have an objection to that.

2. Andrew Bardsley June 24, 2013, 2:06 a.m. (June 24, 2013, 2:06 a.m.)

   Putting some separation between the Tick-ordered EventQueue and one based on counts *would* be nice.  If that 'CounterEventQueue' and the time-based EventQueue didn't share a common base, you might need two Event classes (to accommodate references to EventQueue in Event) which seems a bit unfortunate.
   
   Does anyone have a good feel (from maybe the profiling that gave rise to the Event::nextBin/nextInBin optimisation) for the cost of adding the virtual features like those described in my first comment to the EventQueue/Event actions?  I've not done the experiment but there's significant objection to messing with the structure of EventQueue then I withdraw the above suggestions.
   
   Also, what's the feeling on feature-conditional 'virtual' qualifiers on functions?
   
   #ifdef _SYSTEMC_
   #define SYSTEMC_VIRTUAL virtual
   #else
   #define SYSTEMC_VIRTUAL
   #endif
   
   Is it considered as undesirable as it is ugly?

src/sim/async.hh (Diff revision 1)

It might be useful to have a sim/systemc.hh with this conditional inclusion, maybe the #pragma ... system_header mechanism and any other handy definitions that I'm sure the SystemC interface will gain over time (#include <tlm> for instance!)

Show all issues

This looks very good to me... I'm pleasantly surprised by how modest the changes are.  In addition to addressing the issues that Andrew brought up, I suggest:

1. Adding a few high-level comments describing what's being done.  I'm not a SystemC user, but I can still make some guesses about what's happening; nevertheless, a few brief English sentences in comments here and there would go a long way toward making me confident that my guesses are correct.

2. Wrapping the async event stuff in a common API, so that instead of (for example) having
    #ifdef _SYSTEMC_
        async_event.notify();
    #else
        async_event = true;
    #endif
repeated all over the place, we can have a single set of functions that get called unconditionally, with a minimum number of unique #ifdefs hidden behind that interface.

3. Finding a way other than the 'sysc_build' name to trigger the SystemC compilation.  We already have a mechanism for compile-time configuration flags (the "sticky" vars, like USE_FENV and TARGET_ISA).  Is there a good reason not to just add a 'USE_SYSTEMC' flag there and skip all the 'sysc_build' changes?