> 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. > > Steve Reinhardt wrote: > 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. >
Another "are we naively generalizing for the sake of generalizing?" question that we are so adept at debating :) Although I think it's pretty easy to add the memory model option to the CPU, the point about the "dependencies" between a proposed memory model and the coherence protocol study is a good one. That probably makes the usefulness of a general parameter minimal if its just used in isolation. The "clean" way you (Steve) describe of parameter overrides and warnings would truly be overkill for something that no one would immediately find useful. For the case where there is clearly only two options and a 3rd is far away (e.g. branch delay slots or not, TSO or not, etc.), then I could see how this would be viable. If you get to the point where you have more than two options you are setting/unsetting, then appropriate parameters should be in place. Nilay, I won't stand in the way of your change, it seems reasonable that as long as gem5 is currently "relaxed" or "TSO" that it's not that big of a deal to do this the way you propose. - Korey ----------------------------------------------------------- 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
