> On 2012-01-07 08:43:59, Korey Sewell wrote: > > src/arch/alpha/isa_traits.hh, line 133 > > <http://reviews.m5sim.org/r/908/diff/3/?file=17419#file17419line133> > > > > Guys, I dont think this should be enabled/disabled through the ISA > > TRAITS. > > > > Then, if you are doing any architectural exploration on the impact of > > memory models you are going to have to recompile the code or generate > > different binaries. > > > > Instead, this should be a parameter to the CPU model. The CPU model can > > then choose to implement relaxed ordering, TSO, or whatever flavor someone > > wants. > > > > I dont think there is anything that explicitly ties an ISA to a memory > > model in all cases, so we would be creating a dependency we would regret > > later (for instance, SPARC can have a relaxed model or TSO right?) > > > > And IMO, it will be cleaner to just give the CPU a "memory_model" > > parameter that is a Enum, then the LSQ can by if statement choose what to > > do and then when someone reads the code it will be obvious what's taking > > place and why. > > > > What do people think about that? > > Gabe Black wrote: > I agree with your conclusion and your reasoning. By extension it could be > implemented either be an enum or a collection of bools depending on whether > the models can cleanly be broken down into a set of selectable behaviors like > doing stores one at a time. Then we could avoid things like having if (a || b > || f || g) do_foo(); where a, b, f, and g all have some well defined property > requiring foo.
I agree that someone might want to enable TSO on ARM, or enable SC on x86, or might even come up with a weakly consistent variant of x86. But anyone that's serious about this will probably have a set of specific models they want to compare with, and will be considering the impact of the cache coherence protocol as well, so it's not a trivial thing like increasing the cache size. And how many people are really going to do this? I think what's going on here is clear enough, such that if someone really wanted to do this study they could add a CPU model parameter to override TheISA::HasTSO if they wanted to (or if they weren't that ambitious at coding, they could just change the constant and deal with having two binaries). So sure, ideally it would be great if the CPU model supported all possible consistency models in a cleanly factored way such that each independent behavioral aspect has its own flag, with the default for each ISA being the most aggressive model that that ISA supports, but with parameters exposed to override it with a stronger model or even a weaker model, but making sure to print out adequate warnings in the latter case. If someone actually does this I hope they contribute it back. But I think it's pretty serious overkill to make Nilay do this just to get x86 TSO support in so that x86 O3 MP works correctly. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/908/#review1867 ----------------------------------------------------------- On 2012-01-07 08:11:56, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/908/ > ----------------------------------------------------------- > > (Updated 2012-01-07 08:11:56) > > > Review request for Default. > > > Summary > ------- > > O3 LSQ: Implement TSO > This patch makes O3's LSQ maintain total order between stores. Essentially > only the store at the head of the store buffer is allowed to be in flight. > Only after that store completes, the next store is issued to the memory > system. > > > Diffs > ----- > > src/arch/alpha/isa_traits.hh 93c6317af258 > src/arch/arm/isa_traits.hh 93c6317af258 > src/arch/mips/isa_traits.hh 93c6317af258 > src/arch/power/isa_traits.hh 93c6317af258 > src/arch/sparc/isa_traits.hh 93c6317af258 > src/arch/x86/isa_traits.hh 93c6317af258 > src/cpu/o3/lsq_unit.hh 93c6317af258 > src/cpu/o3/lsq_unit_impl.hh 93c6317af258 > > Diff: http://reviews.m5sim.org/r/908/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
