Is this one commit or two squished together? If it's only one, I'm afraid the meaningful changes will get lost in the merge noise, especially since people (or at least I) don't expect merges to do anything other than merge.

Have you measured how this affects performance? There are a few more touch points than I had imagined, but still not that many. I'd be curious to see if it makes a difference. You could check one ARM and one non-ARM workload (maybe x86, maybe twolf, since it runs relatively quickly) and see how fast they run before and after this change. Be sure to run on a system that isn't doing anything else at the time, and that won't go into weird power management modes half way through.

configs/common/Options.py (Diff revision 1)

Stray comment?

Show all issues

configs/common/Options.py (Diff revision 1)

Stray comment?

Show all issues

configs/example/fs.py (Diff revision 1)

Don't comment it out, delete it.

Show all issues

configs/example/se.py (Diff revision 1)

Not that there's anything necessarily wrong with doing it, but why are you moving this down past the for loop here? It's not obvious how the two would interact.

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   The interaction comes from the way ports are created and connected. When addCheckerCpu is called, it adds 2 ports to the _cache_ports list in BaseCPU for the TLB walkers (if needed).  When the caches are created, it calls connectAllPorts() goes through the _cache_ports list and connects all the CPU ports to the proper bus.  If this happens in the wrong order then the checker ports will end up unconnected and result in a seg-fault.
   
   If desired I can go back and make these two function calls decoupled.

2. Gabe Black Feb. 11, 2012, 4:40 a.m. (Feb. 11, 2012, 4:40 a.m.)

   That doesn't sound like a bad idea, although you don't need to worry about that now I don't think.

src/arch/arm/isa.cc (Diff revision 1)

Is the dynamic_cast necessary? What type does getCheckerCpuPtr return? If you remove it here, remove it later on too.

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   It returns a pointer to the BaseCPU type. The dynamic_cast is technically necessary.  

2. Gabe Black Feb. 11, 2012, 4:40 a.m. (Feb. 11, 2012, 4:40 a.m.)

   The dynamic_cast is basically necessary *if* it returns a BaseCPU pointer. Why does it do that instead of returning a CheckerCPU pointer? Is CheckerCPU really an O3 thing and not a generic class? If it's generic it makes sense to return it directly. If not, maybe something like O3CheckerCPU would be a better name, although renaming it is outside the scope of your change.

3. Geoffrey Blake Feb. 13, 2012, 3:11 a.m. (Feb. 13, 2012, 3:11 a.m.)

   The CheckerCPU is a derived type from the BaseCPU class.  getCheckerCpuPtr() calls the Checker's SimpleThread context with getCpuPtr() which returns a BaseCPU ptr to the checker.  At some point the cast is needed.  I'll make a patch to hide the casts inside the Checker code to make it more explicit what is going on.

src/cpu/SConscript (Diff revision 1)

Delete it, don't comment it out.

Show all issues

src/cpu/SConscript (Diff revision 1)

Is the dummy checker needed any more now that you don't have to have a checker installed?

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   Not sure what you mean here.  The DummyChecker is still needed for proper port creation during configuration for when fast forwarding/checkpointing is used to switch from simple to complex cpus.

2. Gabe Black Feb. 11, 2012, 4:40 a.m. (Feb. 11, 2012, 4:40 a.m.)

   I see. This is so there's hookups to actually switch the real checker into when the O3 takes over.

src/cpu/SConscript (Diff revision 1)

I don't think this check is all that useful any more. I think it would be reasonable to get rid of it entirely.

Show all issues

src/cpu/checker/cpu.cc (Diff revision 1)

You shouldn't make assumptions like this. FS and SE aren't two separate worlds any more, and you can't assume because you have some of the characteristics of one that you'll have all of them. I even have plans specifically for making workload do something in what would have been called FS mode.

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   The SimpleThread object right now assumes a division between FS and SE modes with its two constructors. So for right now its a safe assumption.  When you make those changes I'll re-factor this code.

2. Gabe Black Feb. 11, 2012, 4:40 a.m. (Feb. 11, 2012, 4:40 a.m.)

   Ok

src/cpu/checker/cpu.cc (Diff revision 1)

Does this actually need to be FullSystem only?

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   Yes, because otherwise two SimpleThread objects will be created.  If we changed the SimpleThread object to have one constructor instead of two where one is just for FS and one for SE configs (the two constructors take different args and do different things) this should go away.

2. Gabe Black Feb. 11, 2012, 4:40 a.m. (Feb. 11, 2012, 4:40 a.m.)

   Ok

src/cpu/checker/cpu_impl.hh (Diff revision 1)

Why does this need to be FullSystem only?

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   Yes.  In SE mode faults just happen and no PC change occurs right?  Only in FS will the PC be vectored to a fault handler.

2. Gabe Black Feb. 11, 2012, 4:40 a.m. (Feb. 11, 2012, 4:40 a.m.)

   No. In SE faults *usually* just happen (typically panicing or updating the TLB), but that's not a hard rule. In SPARC specifically there are faults that do register window spills and fills in SE mode in actual handlers injected into memory before the process starts.

3. Geoffrey Blake Feb. 13, 2012, 3:11 a.m. (Feb. 13, 2012, 3:11 a.m.)

   That's a good bit of info to know if someone wants to use the Checker with the SPARC ISA.  But since I'm only worried about it working with ARM for now I'll put this off to the side right now.

4. Gabe Black Feb. 13, 2012, 10:07 a.m. (Feb. 13, 2012, 10:07 a.m.)

   I don't think it's a good idea to build in an assumption like that, especially since it shouldn't be that hard to fix (you can actually check if the PC changed once you invoke the fault). As a matter of fact, to further unify SE and FS, I'm planning to make all faults/syscalls in all ISAs actually use the normal mechanisms to vector tables/whatever injected into memory which hold pseudo ops that cause the emulated behavior, panics, etc.

5. Geoffrey Blake Feb. 17, 2012, 2:15 a.m. (Feb. 17, 2012, 2:15 a.m.)

   Part of the problem right now is that in SE mode the fault will be re-executed, and not all syscalls and fault handlers do idempotent operations.  This will have to be fixed with some sort of logic that won't let the fault do something to the state if its called from the Checker.  I'm not sure what should be done for this. This problem is avoided in FS operation because the Checker never updates memory.

6. Gabe Black Feb. 17, 2012, 5:52 a.m. (Feb. 17, 2012, 5:52 a.m.)

   Yeah, that's a fairly tricky problem, and leaving that in the if is probably the best thing to do for now.

src/cpu/simple/BaseSimpleCPU.py (Diff revision 1)

A checker on the simple CPU doesn't seem to make any sense. This could error out all the time, and it should set the checker to NULL since that's the inert setting.

Show all issues

1. Geoffrey Blake Feb. 10, 2012, 7:14 a.m. (Feb. 10, 2012, 7:14 a.m.)

   If the function addCheckerCpu() isn't called then no checker is added to the cpu model.   See the above comment as to why a dummy checker is still needed.

Made style changes in regards to commented out code, and dead code.  I have yet to perform performance tests.  Will get to that ASAP and post test results here.

Removed the need for the dynamic_cast to the CheckerCPU object in the code.  Performed performance impact tests and noted a negligible impact.

Rebased against the most recent changes to the source tree.

Rebasing for recent changes.

src/cpu/checker/cpu.cc (Diff revision 5)

Don't just comment code out!

Show all issues

src/cpu/checker/thread_context.hh (Diff revision 5)

Should it not be a dynamic_cast rather?

Show all issues

1. Nathan Binkert Feb. 29, 2012, 12:22 a.m. (Feb. 29, 2012, 12:22 a.m.)

   If you know that it should be a CheckerCPU, then a static cast is certainly faster.  You can use a safe_cast (which is in base/cast.hh) which does an assertion on the cast to make sure that it was valid.

Made style changes and other suggested corrections.

Ship It!