> On Feb. 1, 2012, 8:39 a.m., Steve Reinhardt wrote: > > src/cpu/o3/commit_impl.hh, line 1004 > > <http://reviews.gem5.org/r/1018/diff/1/?file=21617#file21617line1004> > > > > Why is there a check for tid == 0 here? > > > > Also, where does the Alpha pal-mode check come from? I don't see it in > > the original code. > > > > Nilay Vaish wrote: > Interrupts can only be handled by the thread with tid 0, so this > check is required. I think we can elide this check, if assume that > number of threads will always be set to 1. The Alpha pal-mode check > has been copied from the fetch stage. It might be possible that the > committed microop is marked non-delayed but is still a microop in the > pal-code. In this, case we do not want to handle the interrupt now. > > Steve Reinhardt wrote: > Where does the tid 0 restriction come from? It's probably moot since I > don't know of any ISAs for which we support SMT in FS mode, but it seems odd > that a "new" restriction is being introduced here in commit that wasn't here > previously. Similarly for the Alpha pal-mode check... why is it necessary to > do this in commit now when it wasn't necessary before? > > > Nilay Vaish wrote: > Given my understanding of the code, there is no new restriction > being introduced. Interrupts were always handled using thread 0. > We any way need some check to ensure that the last microop of the > thread that handles the interrupt was marked non-delayed. It is > possible to introduce an array of booleans to maintain per thread > flag. But that seems unnecessary if only thread 0 is going to handle > the interrupt. > > The pal-mode check is necessary since interrupts cannot be handled > in pal-mode. It was not there before does not mean that what ever > was happening before was correct. Even the check for delayed commit > was not there before. By this logic, the entire patch would seem > unnecessary. > > Steve Reinhardt wrote: > I'm saying "new" because this code is appearing seemingly out of nowhere > with respect to this patch. If these are restrictions you're copying from > somewhere else, that's fine, but where is that other code? Is it still > necessary in that other place? And if we're going to have conditions like > this in multiple places, would it be a good idea to capture them in a > function that gets called from both places rather than having the logic > duplicated in a way that makes it prone to getting changed in one place and > not the other? >
The code as such appears in 2 or 3 functions. If you read the file commit_impl.hh, you would find that the there are places where certain conditions are being set / checked only for thread 0. It seems odd as to why one may want to prefer thread 0 for handling interrupts or differentiate amongst threads. It might be that there are some problems with handling interrupt with other threads, though I am not aware of any. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1018/#review2030 ----------------------------------------------------------- On Jan. 28, 2012, 9:01 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1018/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2012, 9:01 p.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 8732:75e54d141aa0 > --------------------------- > O3 CPU: Strengthen condition for handling interrupts > The condition for handling interrupts is to check whether or not the cpu's > instruction list is empty. As observed, this can lead to cases in which even > though the instruction list is empty, interrupts are handled when they should > not be. The condition is being strengthened so that interrupts get handled > only > when the last committed microop did not had IsDelayedCommit set. > > > Diffs > ----- > > src/cpu/o3/commit.hh 9d7c1dc54954 > src/cpu/o3/commit_impl.hh 9d7c1dc54954 > src/cpu/o3/fetch_impl.hh 9d7c1dc54954 > > Diff: http://reviews.gem5.org/r/1018/diff/diff > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
