> 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.
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? - Steve ----------------------------------------------------------- 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
