In generalI think this is the kind of ISA hook we should be using... in the sense that checking TheISA::HasUnalignedMemAcc is much better than "(TheISA == x86 || TheISA == Power)". I think it's useful not only to avoid the overhead of a dynamic check for an ISA that doesn't need it, but also to clarify that this is code that never gets executed on those ISAs. Maybe for a little one-liner like this it's not a big deal either way, but for bigger hunks of code I think that clarification is potentially useful.
Steve On Thu, Jul 22, 2010 at 1:10 PM, Gabriel Michael Black <gbl...@eecs.umich.edu> wrote: > Yes it does, and that sounds reasonable to me. I'd still like to see us use > ISA hooks as minimally as possible, but this seems ok. > > Gabe > > Quoting Timothy M Jones <tjon...@inf.ed.ac.uk>: > >> Oh, ok, I see where you're going with that. However, the main idea of >> having TheISA::HasUnalignedMemAcc was that it is a constant specific to each >> ISA. Therefore, the compiler should really recognise this and optimise it >> away wherever it's used. In this case, for ISAs that don't have unaligned >> memory accesses the whole 'if' block should disappear. For ISAs that do >> have them then the condition should be reduced to just checking sreqLow. >> Therefore it's better for the first set of ISAs for this to be kept in. >> Does that make sense? >> >> Tim >> >> On Thu, 22 Jul 2010 14:30:56 -0400, Gabe Black <gbl...@eecs.umich.edu> >> wrote: >> >>> I think you missed my point. If the check of TheISA::HasUnalignedMemAcc >>> is redundant, we shouldn't be checking it at all. It's a free, though >>> very small, performance bump, but more significantly it removes a direct >>> dependence on the ISA. >>> >>> Gabe >>> >>> Timothy M. Jones wrote: >>>> >>>> changeset 3bd51d6ac9ef in /z/repo/m5 >>>> details: http://repo.m5sim.org/m5?cmd=changeset;node=3bd51d6ac9ef >>>> description: >>>> O3CPU: Fix a bug where stores in the cpu where never marked as >>>> split. >>>> >>>> diffstat: >>>> >>>> src/cpu/o3/lsq_unit.hh | 6 ++++++ >>>> 1 files changed, 6 insertions(+), 0 deletions(-) >>>> >>>> diffs (16 lines): >>>> >>>> diff -r 02b471d9d400 -r 3bd51d6ac9ef src/cpu/o3/lsq_unit.hh >>>> --- a/src/cpu/o3/lsq_unit.hh Thu Jul 22 18:47:52 2010 +0100 >>>> +++ b/src/cpu/o3/lsq_unit.hh Thu Jul 22 18:52:02 2010 +0100 >>>> @@ -822,6 +822,12 @@ >>>> storeQueue[store_idx].sreqLow = sreqLow; >>>> storeQueue[store_idx].sreqHigh = sreqHigh; >>>> storeQueue[store_idx].size = sizeof(T); >>>> + >>>> + // Split stores can only occur in ISAs with unaligned memory >>>> accesses. If >>>> + // a store request has been split, sreqLow and sreqHigh will be >>>> non-null. >>>> + if (TheISA::HasUnalignedMemAcc && sreqLow) { >>>> + storeQueue[store_idx].isSplit = true; >>>> + } >>>> assert(sizeof(T) <= sizeof(storeQueue[store_idx].data)); >>>> >>>> T gData = htog(data); >>>> _______________________________________________ >>>> m5-dev mailing list >>>> m5-dev@m5sim.org >>>> http://m5sim.org/mailman/listinfo/m5-dev >>>> >>> >>> _______________________________________________ >>> m5-dev mailing list >>> m5-dev@m5sim.org >>> http://m5sim.org/mailman/listinfo/m5-dev >>> >> >> >> -- >> Timothy M. Jones >> http://homepages.inf.ed.ac.uk/tjones1 >> >> The University of Edinburgh is a charitable body, registered in >> Scotland, with registration number SC005336. >> >> _______________________________________________ >> m5-dev mailing list >> m5-dev@m5sim.org >> http://m5sim.org/mailman/listinfo/m5-dev >> > > > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
