> On May 27, 2015, 4:21 p.m., Joel Hestness wrote: > >
Thanks for the insightful comments Joel! > On May 27, 2015, 4:21 p.m., Joel Hestness wrote: > > src/arch/x86/isa/microops/specop.isa, line 70 > > <http://reviews.gem5.org/r/2846/diff/1/?file=45418#file45418line70> > > > > Can you please set flags consistently for this instruction? Either move > > this up into the X86MicroopBase constructor call, or move the setFlags bits > > (IsNonSpeculative, IsQuiesce) down into the body here to be consistent with > > this line of code? I don't see any other examples of flag setting in > > micro-op code, but x86 macro-op code seems to follow the latter convention. Yeah, having them set in the MicroHalt constructor instead of in the constructor call of X86MicroopBase seems cleaner as well. Thanks for catching this! > On May 27, 2015, 4:21 p.m., Joel Hestness wrote: > > src/cpu/o3/fetch_impl.hh, line 996 > > <http://reviews.gem5.org/r/2846/diff/1/?file=45424#file45424line996> > > > > Removing this assertion seems to be a symptom of changing the > > abstraction for communicating stalls between pipe stages. Hypothetically, even if DefaultFetch::doSquash() resets the stall signal from decode, there still is a situation when decode is unblocked and there is no need to assert the decode stall signal. > On May 27, 2015, 4:21 p.m., Joel Hestness wrote: > > src/cpu/o3/cpu.cc, line 754 > > <http://reviews.gem5.org/r/2846/diff/1/?file=45420#file45420line754> > > > > Is there a reason not to include code to clear stalls within > > fetch.squash() and decode.squash()? The stalls[tid] arrays are only > > accessed within their respective pipe stages and set/reset based on signals > > from adjacent pipe stages. Exposing them publicly through the removeStall() > > functions seems like it breaks the existing pipe stage stall signalling > > abstraction. It seems that there are other stall signals exposed publicly (e.g. drainStall()). The main reason these can not go into the squash method is that there are scenarious when squashes w/ decode and fetchs stall signals enabled seem to be required. This seems a bit unintuitive and haven't digged into why this is happening, however having these stall signals reset in squash causes the pipeline to stop executing at some point. - Alexandru ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2846/#review6398 ----------------------------------------------------------- On May 26, 2015, 4:45 p.m., Alexandru Dutu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2846/ > ----------------------------------------------------------- > > (Updated May 26, 2015, 4:45 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10858:2fb10c49dd93 > --------------------------- > cpu: o3: Merging haltContext with suspendContext > This patch improves suspendContext by flushing the pipeline, which frees > resources for other hardware threads. Secondly, it makes haltContext call > suspendContext which is not freeing architectural registers mappings on halt. > Something that haltContext previously did and was not realistic. > For example, in SMT implementations the architectural registers for a > particular hardware thread will always have mapped some physical registers and > having one thread finish execution will never create more available physical > registers for other hardware threads as there will be scheduled a different > software thread to execute on that hardware thread anyway. > As a consequence, this patch helps enabling SMT in x86 by not putting the > physical registered mapped to the ZeroRegister on the freeList for a different > thread to pick up when one of the threads has finished executing and called > exit. > > > Diffs > ----- > > src/cpu/o3/fetch_impl.hh d02b45a554b52c68cce41e1b3895fb8582a639dd > src/arch/x86/isa/microops/specop.isa > d02b45a554b52c68cce41e1b3895fb8582a639dd > src/cpu/o3/cpu.hh d02b45a554b52c68cce41e1b3895fb8582a639dd > src/cpu/o3/cpu.cc d02b45a554b52c68cce41e1b3895fb8582a639dd > src/cpu/o3/decode.hh d02b45a554b52c68cce41e1b3895fb8582a639dd > src/cpu/o3/decode_impl.hh d02b45a554b52c68cce41e1b3895fb8582a639dd > src/cpu/o3/fetch.hh d02b45a554b52c68cce41e1b3895fb8582a639dd > > Diff: http://reviews.gem5.org/r/2846/diff/ > > > Testing > ------- > > Quick regressions passed. > > > Thanks, > > Alexandru Dutu > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
