> On 2012-01-01 16:51:13, Ali Saidi wrote: > > I've got a couple of issues with this change. The first is to do with the > > general direction. You're solving the problem here by removing the > > communication delay from the interrupt pending signal to the processing of > > the interrupt. In CPUs there really is a delay for this to propagate, so > > it's not clear to me that we want to get rid of it. Furthermore, once the > > variables you've added are set, I don't see a way for them to be cleared. > > With the previous code the time buffers clear them, but with the new code > > they're only ever set, so it seems like this couldn't work. Finally, your > > commit message needs to have information about when this problem shows up, > > how you addressed it, and what you believe is now fixed. > > > > Thanks, > > Ali > > > > Nilay Vaish wrote: > Ali, is there a particular reason that the commit stage of processor > would transmit this information of pending interrupt to the fetch > stage? I admit I don't know how this is done in a processor. Is there > not a common line which any stage can read from, and not that a > particular stage informs others that an interrupt is pending? > > In case of clear interrupt, I agree with you that there should > be a delay between the processing of the interrupt and this > information being received by the fetch stage. > > Gabe Black wrote: > There are a number of things that are off with change and, in my opinion, > how O3 is handling interrupts. First, as I've brought up in the past, I think > making commit decide to accept an interrupt, passing that back to fetch, and > trusting fetch to turn off the tap while the interrupt should still be > recognized, possibly stepping over a small window intended to allow > interrupts, is overly complicated and not likely to work correctly all the > time except maybe with Alpha where special circumstances apply. I think we > should get rid of that entirely, let commit decide when interrupts happen > with the existing ISA specific objects, and if one should happen nuke the > pipe like a fault. > > If a macroop has partially executed, none of its microops are necessarily > in flight, although most of the time some would be. O3 checking that the > instList is empty, if it is indeed doing that, sounds broken. There should be > exactly two conditions for recognizing and processing an interrupt, did the > last instruction have isDelayedCommit cleared, and did the interrupts object > say that by the rules defined by the ISA (interrupt enable bits, priority > levels, whether an interrupt was signaled and how, etc.). Whether > instructions are in flight should be irrelevant, although there could be > something wonky in place right now to make the system of sending a signal to > fetch work. > > Executing the last microop is not necessary or sufficient to recognize an > interrupt. Some instructions in x86 have an interrupt shadow and suppress > interrupts immediately after them, beyond the end of the macroop, and some > (string instructions) can be interrupted at certain points while they > execute. The instructions are supposed to look like they repeat over a string > of data, but really they're implemented as a loop in microcode. > isDelayedCommit is set when interrupts are not allowed after an instruction > and cleared when they are and is highly correlated with isLastMicroop but not > interchangeable with it. > > Ali Saidi wrote: > I would like to point out again that I can't find any way that > cpu->clearInterrupt or cpu->interruptPending will ever be set to false after > they're set to true.
Ali, I am aware of your concern and I would not be committing this patch. I agree that what I am trying to do in that patch is incorrect. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/960/#review1792 ----------------------------------------------------------- On 2011-12-30 16:31:40, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/960/ > ----------------------------------------------------------- > > (Updated 2011-12-30 16:31:40) > > > Review request for Default. > > > Summary > ------- > > O3 CPU: Remove interruptPending from fetch > The fetch stage currently has a variable for tracking whether interrupts > are pending are not. This variable's value is communicated from the commit > stage. Instead, the variable is being moved to the O3CPU class and will > accessed using the pointer to the CPU. > > > Diffs > ----- > > src/cpu/o3/comm.hh ca98021c3f96 > src/cpu/o3/commit_impl.hh ca98021c3f96 > src/cpu/o3/cpu.hh ca98021c3f96 > src/cpu/o3/cpu.cc ca98021c3f96 > src/cpu/o3/fetch.hh ca98021c3f96 > src/cpu/o3/fetch_impl.hh ca98021c3f96 > > Diff: http://reviews.m5sim.org/r/960/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
