O3 CPU: Remove interruptPending from fetch

Information
Submitter:	Nilay Vaish
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

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.

Testing Done

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 Jan. 1, 2012, 10:09 a.m. (Jan. 1, 2012, 10:09 a.m.)

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 Jan. 1, 2012, 2:28 p.m. (Jan. 1, 2012, 2:28 p.m.)

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 Jan. 3, 2012, 2:41 a.m. (Jan. 3, 2012, 2:41 a.m.)

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.

Nilay Vaish Jan. 3, 2012, 3:23 a.m. (Jan. 3, 2012, 3:23 a.m.)

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.

src/cpu/o3/commit_impl.hh (Diff revision 1)

It's not clear how this would ever be cleared after it's initially set.

src/cpu/o3/commit_impl.hh (Diff revision 1)

It's not clear to me how this would ever be cleared after it was initially set

src/cpu/o3/cpu.hh (Diff revision 1)

These aren't doxygen comments.

src/cpu/o3/fetch.hh (Diff revision 1)

I don't know that you would need clearInterrupt at all if you implemented the code like this.

You have a pending review.

Review Board 2.0.15

This change has been discarded.

Screenshots

Files

Status: Discarded