> On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/faults.hh, line 113 > > <http://reviews.m5sim.org/r/20/diff/1/?file=175#file175line113> > > > > Seems redundant to have 'Arm' in the name of a class that's inside the > > 'ArmISA' namespace... I can see where there might be some confusion if > > there's a base Fault that's outside the namespace as well, but maybe that's > > a sign that there's a larger renaming that should be done. > > > > Also, Fault:FaultBase seems more consistent with other naming than > > FaultVals:Fault; I'm curious what motivated the change. > > > > Gabe Black wrote: > It looks like I made that change. I don't really remember my motivation > (a danger of putting off reviews for so long), but I think it was probably > that ArmFaultVals is the class that bolts on the static FaultVals struct and > accessors for it for the curious recurring template pattern (crtp). Changing > ArmFaultBase to ArmFault was likely because the Base is sort of redundant > when ArmFault is sufficiently general enough by itself. I think part of the > reason it might look upside down is that basically (no pun intended) that's > the way the crtp forces you to do things. > > As far as the Arm part of the name, I think that's just historically how > it's been set up with SPARC, Alpha, etc. I definitely don't remember why I > did that, but I think it was similar to what you said, to avoid ambiguity as > far as whether the class was general or ISA specific. There is no generic > Fault class (only FaultBase) so it's probably not strictly necessary. I > wouldn't mind if we changed it, it's just that nobody brought it up before. > > Steve Reinhardt wrote: > It could be that the historical inclusion of the ISA name in the fault > class is that originally we may not have used the ISA namespace...
Ah yes, mercurial backs that up. I'm surprised you remembered that :). It'd be neat to look back that far in the history (more thoroughly than I just did) and see again how everything grew up. It's fun to see how different some things were. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/mem.hh, line 252 > > <http://reviews.m5sim.org/r/20/diff/1/?file=181#file181line252> > > > > seems like this should go in a .cc file > > Gabe Black wrote: > Wouldn't that prevent it from being inlined? Or do we not care since it's > disassembly? > > Steve Reinhardt wrote: > We don't care... disassembly is never performance critical to begin with, > and we only call the methods at most once per StaticInst anyway (the return > string is cached and reused after that). This may not be the best place to mention it, but there's a glitch in that that the Florida people had at least attempted to address, although I wiped out their fix in these changes. Basically, -most- of the disassembly is constant, but the pc value may change between uses as far as I can tell and it won't invalidate the disassembled string. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/vfp.hh, line 314 > > <http://reviews.m5sim.org/r/20/diff/1/?file=190#file190line314> > > > > What's the purpose of these __asm__ statements? > > > > Gabe Black wrote: > gcc was reordering setting fp rounding modes after the operation I needed > them set for, extracting condition flags before them, etc. It was generally a > mess. These say "Look out gcc! You can't see what I'm doing so you better be > careful!" and it puts things were they're supposed to be. I think there's > supposed to be a way to do this without clubbing the compiler over the head, > but it isn't actually supported properly in gcc. > > Steve Reinhardt wrote: > Interesting... do you need to declare some variables as volatile? It > seems like that ought to be sufficient, and of course would be a lot more > portable. If I'm remembering right I tried that and it didn't work. I think by using __volatile__ __asm__ you get stronger guarantees, maybe because they inseparably both consume and produce the value at a certain point. One nice-ish property of the asm route is that you sort of turn on the volatileness at the critical points and let it slip back into a register later. These are sort of portable since they don't actually have any code in them, although they do mean the compiler you're working with needs to support that sort of inline asm syntax. We do have gcc dependencies elsewhere, too. I'm not a huge fan of these (they were about the 5th attempt), but they seemed to be the only way to get things to work. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/isa/formats/data.isa, line 41 > > <http://reviews.m5sim.org/r/20/diff/1/?file=206#file206line41> > > > > Is there a reason we can't use predefined bitfields here (and a ton of > > other places)? > > Gabe Black wrote: > ARM defines bitfields like movie theaters make popcorn. There was a huge > list in ExtMachInst (or the isa description, if you prefer) which all had > nearly meaningless names and which still only covered a small fraction of > what was needed. Usually the names are ambiguous (globally at least) and > nearly meaningless even in the manual, ie. things like "op", "op1", "u", "a", > "b", "c", etc. By just extracting most of them in place they stay meaningless > and arbitrary, but they don't spill into a global context and don't end up in > a giant list. There -are- cases, I'm sure, were something wasn't ambiguous, > had a real name, and I decoded it this way anyway. I hope those are very rare. > > Steve Reinhardt wrote: > OK, this is the other bitfield issue I raised above. It seems like I did > see some spots where these appeared to be redundant wrt the defined > bitfields. Even when that's not the case, it would be nice to get rid of the > redundancy where we have "bits(machInst, X, Y)" with the same X and Y > appearing in multiple places and referring to the same logical field. My > preference would be to have the bitfields defined and labeled somewhere even > if there are a bazillion of them, maybe with the possible exception of the > truly one-off fields; to a large extent this is a corollary of the general > "no magic constants" rule. > I'm pretty sure (although not entirely sure) that when X and Y are the same it's coincidence. There are very few places that I can recall where the placement of bitfields or their names seemed to have any greater purpose than that was just an unambiguous (strictly speaking) place to stuff some bits. I think if you looked through the manual you'll see that most bitfields are one-off which was actually pretty frustrating while working on the decoder. Going with the magic number analogy, It'd be like having a file filed with #define ONE 1 #define TWO 2 to avoid using magic numbers like 1 and 2. The problem is if they actually -are- arbitrary, meaningless values, giving them arbitrary meaningless names doesn't make them any less magical. As far as what's there right now, I think things are probably not quite right in both directions. There are probably a few (rare, at least I hope) instances of bitfields with actual significance not being given a real name, and some bitfields that were still in use by code that's since been replaced with meaningless names like these def bitfield TOPCODE_15_13 topcode15_13; def bitfield TOPCODE_13_11 topcode13_11; def bitfield TOPCODE_12_11 topcode12_11; def bitfield TOPCODE_12_10 topcode12_10; def bitfield TOPCODE_11_9 topcode11_9; def bitfield TOPCODE_11_8 topcode11_8; def bitfield TOPCODE_10_9 topcode10_9; def bitfield TOPCODE_10_8 topcode10_8; OPCODE is even a misnomer here. These are nothing more than bits X_Y of a 16 bit thumb instruction. The old version of things and older, pre thumb versions of the architecture make a passing attempt at named, consistent bitfields, although it wasn't perfect. I tried extending that to the complete, modern version of the ISA, but it fell apart when they'd move around, get recycled, etc. so I went to what's above. Those were tedious and frustrating to work with, so I just gave up and grabbed the bits I needed when I needed them. Even the manual backed off of some fields that used to have more descriptive names. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/isa/decoder/decoder.isa, line 44 > > <http://reviews.m5sim.org/r/20/diff/1/?file=201#file201line44> > > > > It would be clearer to do: > > > > decode THUMB default Unknown::unknown() { > > 0: ##include "arm.isa" > > 1: ##include "thumb.isa" > > } > > > > then get rid of the context-free 0:/1: in the included files... > > (add line breaks if necessary if we require the '##' to be at the > > beginning of a line) > > Gabe Black wrote: > I agree, but I don't think that works for some reason. I think it > confuses the parser. ## doesn't have to be at the beginning of a line as far > as I remember. It would be nice to fix that. Actually, thinking about it just > now, I remember ##include was replaced by something to tell the parser you're > in a new file so it can print sensible error messages. That doesn't actually > work right (I fix it in one place, it breaks in another :( very frustrating) > but in any case I'm guessing the extra tokens mid construct prevents the > grammar from matching up. > > Steve Reinhardt wrote: > Yea, looking at the code it looks like we require ##include to be the > first non-whitespace on a line, and we emit this '##newfile/##endfile' pair > to delineate the included text (which look like they're not actually used... > is that true?). I'm guessing this issue could be fixed by adding more > newlines when we insert those tokens and/or not requiring newlines when we > scan for them. They're used around here http://repo.m5sim.org/m5/file/24379f92cc10/src/arch/isa_parser.py#l1197 . Maybe dropping the "^" would help. I just accepted the flakiness long ago so I don't remember all the details of why things are the way they are. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/isa/bitfields.isa, line 49 > > <http://reviews.m5sim.org/r/20/diff/1/?file=197#file197line49> > > > > I assume these are all instances of your C++ bitfield class? Maybe we > > should add some first-class support in the ISA parser to get rid of this > > redundant-looking header. > > What are the advantages of doing it this way vs. the old way, anyway? > > Gabe Black wrote: > They aren't instances of the C++ bitfield class (BitUnion, coined by Nate > I think), but the ExtMachInst is and so these are members of one. First class > support was added to the parser for x86 where ExtMachInst is actually a > struct, some members of which are BitUnions. Basically, if the descriptive > part of a def bitfield is a name, it's interpreted as a named field of the > ExtMachInst. encoding would turn into a macro defined as machInst.encoding. > This is more important in x86 where there's simply no other way to get at > structure members and the ExtMachInst is too big for anything else. It adds > value here too since it removes redundancy between the ISA definition and > parts outside of there that need to refer to bitfields, for instance the > predecoder. The ExtMachInst type defines everything to be where it needs to > be, and then the ISA description, predecoder, whatever, can get at what it > needs using a named bitfield and not a convention of bit positions. This is > important in the predecoder because it defines a few artificial instruction bits to contextualize an instruction (arm vs. 32 bit thumb vs. 16 bit thumb, etc.) and also to the load/store multiple constructor where most of the decoding for those instructions take place. It's a lot easier to have those constructed programatically as needed than to have all the versions around and to pick the right one. That was something the Florida guys had done originally, and it seemed like the right way to continue to do it. > > If you're suggesting replacing -all- instances of the bitfield > definitions with the BitUnion stuff, that would make sense to me. It would > get rid of some macros and streamline the parser a bit, although the > BitUnions themselves are a little magical too. One important bug in the > BitUnions that we (or really I) have yet to solve is that if you assign one > that's of the same signedness, same position in its parent, and from a parent > of the same type to another, gcc sees that they're the same type and just > copies one on top of the other rather than going through a regular scalar > intermediary. The Bitfields are really all unioned on top of each other and > "contain" the data for the whole BitUnion, so in that case you'll overwrite > the whole value with the other rather than just the Bitfield you thought you > were. I'm sure (or really really hope) there's some C++-foo that can be > applied to correct that, but I wasn't able to find anything that made gcc > happy and worked correctly the last I tried. > > Steve Reinhardt wrote: > There are two things that seem sub-optimal from my 5,000-ft view: > > 1. A file containing a bunch of lines that just say "def bitfield FOO > foo;" for various values of "foo" > 2. The fact that there are still a bunch of expressions like > "bits(machInst, X, Y)" with magic values of X and Y in the C++ part of the > decode logic, and these are frequently redundant in that there are multiple > instances of the same expression with identical values of X and Y. > > Maybe these are unrelated, but they're both dealing with bitfields, which > is why they stay close together in my head. To me it would be best if both > could be made to go away. > > For the first one, instead of having an exception where if a bitfield > refers to a string rather than an actual bitfield expression we assume it's > an ExtMachInst field, why not just assume that anywhere in the grammar you > expect a defined bitfield (via def bitfield) but have an identifier instead > then it must be an ExtMachInst field? I'd be more sympathetic toward the > former if it gave better error messages, but (1) I suspect that may not be > true and (2) even if it is, there's probably a cleaner way to do it (e.g., > allow something like "def bitfields foo, bar, baz;" which is equivalent to > "def bitfield foo foo;" etc. > > For the second one, I need you to explain more why it is the way it is to > begin with (unless that's in another comment somewhere that I haven't read > yet). As far the second strategy you mention for 1, I don't like how that would effectively make a macro all lower case. It also collapses a namespace where you might have a field called, say, n or i or x, and preclude having those as variable names. You might say i is a lousy name for a bitfield and you'd be right, but that's only an easy example. I'm sure there are plenty of other more defensible ways to run into trouble. That's mitigated if you toupper everything, but that feels like a hack. The first strategy is better since it avoids all that, but then it precludes using the macros in the actual C++ code. I can't think of any place that's essential, but it is handy. I wouldn't mind getting rid of def bitfield entirely since that would gets rid of some macro magic (generated macro magic at that), but I don't have a better idea (I like to complain, not fix things!). I talk more about 2 below. - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/20/#review15 ----------------------------------------------------------- On 2010-04-29 15:33:58, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/20/ > ----------------------------------------------------------- > > (Updated 2010-04-29 15:33:58) > > > Review request for Default. > > > Summary > ------- > > Initial set of patches to improve the M5 support of the ARM ISA. Bundled into > one large change for review. This change implements the majority of thumb, > thumb2, and arm instructions and allows the running of all tested SPEC2000 > benchmarks in atomic mode. > > > Diffs > ----- > > configs/common/cpu2000.py ad784e759a74 > src/arch/arm/ArmTLB.py ad784e759a74 > src/arch/arm/SConscript ad784e759a74 > src/arch/arm/faults.hh ad784e759a74 > src/arch/arm/faults.cc ad784e759a74 > src/arch/arm/insts/branch.hh ad784e759a74 > src/arch/arm/insts/branch.cc ad784e759a74 > src/arch/arm/insts/macromem.hh ad784e759a74 > src/arch/arm/insts/macromem.cc PRE-CREATION > src/arch/arm/insts/mem.hh ad784e759a74 > src/arch/arm/insts/mem.cc ad784e759a74 > src/arch/arm/insts/misc.hh PRE-CREATION > src/arch/arm/insts/misc.cc PRE-CREATION > src/arch/arm/insts/mult.hh PRE-CREATION > src/arch/arm/insts/pred_inst.hh ad784e759a74 > src/arch/arm/insts/pred_inst.cc ad784e759a74 > src/arch/arm/insts/static_inst.hh ad784e759a74 > src/arch/arm/insts/static_inst.cc ad784e759a74 > src/arch/arm/insts/vfp.hh PRE-CREATION > src/arch/arm/insts/vfp.cc PRE-CREATION > src/arch/arm/interrupts.hh ad784e759a74 > src/arch/arm/interrupts.cc ad784e759a74 > src/arch/arm/intregs.hh ad784e759a74 > src/arch/arm/isa.hh ad784e759a74 > src/arch/arm/isa.cc PRE-CREATION > src/arch/arm/isa/bitfields.isa ad784e759a74 > src/arch/arm/isa/copyright.txt ad784e759a74 > src/arch/arm/isa/decoder.isa ad784e759a74 > src/arch/arm/isa/decoder/arm.isa PRE-CREATION > src/arch/arm/isa/decoder/decoder.isa PRE-CREATION > src/arch/arm/isa/decoder/thumb.isa PRE-CREATION > src/arch/arm/isa/formats/basic.isa ad784e759a74 > src/arch/arm/isa/formats/branch.isa ad784e759a74 > src/arch/arm/isa/formats/breakpoint.isa PRE-CREATION > src/arch/arm/isa/formats/data.isa PRE-CREATION > src/arch/arm/isa/formats/formats.isa ad784e759a74 > src/arch/arm/isa/formats/fp.isa ad784e759a74 > src/arch/arm/isa/formats/macromem.isa ad784e759a74 > src/arch/arm/isa/formats/mem.isa ad784e759a74 > src/arch/arm/isa/formats/misc.isa PRE-CREATION > src/arch/arm/isa/formats/mult.isa PRE-CREATION > src/arch/arm/isa/formats/pred.isa ad784e759a74 > src/arch/arm/isa/formats/uncond.isa PRE-CREATION > src/arch/arm/isa/formats/unimp.isa ad784e759a74 > src/arch/arm/isa/formats/unknown.isa ad784e759a74 > src/arch/arm/isa/formats/util.isa ad784e759a74 > src/arch/arm/isa/includes.isa ad784e759a74 > src/arch/arm/isa/insts/basic.isa PRE-CREATION > src/arch/arm/isa/insts/branch.isa PRE-CREATION > src/arch/arm/isa/insts/data.isa PRE-CREATION > src/arch/arm/isa/insts/div.isa PRE-CREATION > src/arch/arm/isa/insts/fp.isa PRE-CREATION > src/arch/arm/isa/insts/insts.isa PRE-CREATION > src/arch/arm/isa/insts/ldr.isa PRE-CREATION > src/arch/arm/isa/insts/macromem.isa PRE-CREATION > src/arch/arm/isa/insts/mem.isa PRE-CREATION > src/arch/arm/isa/insts/misc.isa PRE-CREATION > src/arch/arm/isa/insts/mult.isa PRE-CREATION > src/arch/arm/isa/insts/str.isa PRE-CREATION > src/arch/arm/isa/insts/swap.isa PRE-CREATION > src/arch/arm/isa/main.isa ad784e759a74 > src/arch/arm/isa/operands.isa ad784e759a74 > src/arch/arm/isa/templates/basic.isa PRE-CREATION > src/arch/arm/isa/templates/branch.isa PRE-CREATION > src/arch/arm/isa/templates/macromem.isa PRE-CREATION > src/arch/arm/isa/templates/mem.isa PRE-CREATION > src/arch/arm/isa/templates/misc.isa PRE-CREATION > src/arch/arm/isa/templates/mult.isa PRE-CREATION > src/arch/arm/isa/templates/pred.isa PRE-CREATION > src/arch/arm/isa/templates/templates.isa PRE-CREATION > src/arch/arm/isa/templates/vfp.isa PRE-CREATION > src/arch/arm/isa_traits.hh ad784e759a74 > src/arch/arm/linux/linux.hh ad784e759a74 > src/arch/arm/linux/process.hh ad784e759a74 > src/arch/arm/linux/process.cc ad784e759a74 > src/arch/arm/miscregs.hh ad784e759a74 > src/arch/arm/miscregs.cc PRE-CREATION > src/arch/arm/nativetrace.cc ad784e759a74 > src/arch/arm/pagetable.hh ad784e759a74 > src/arch/arm/pagetable.cc ad784e759a74 > src/arch/arm/predecoder.hh ad784e759a74 > src/arch/arm/process.hh ad784e759a74 > src/arch/arm/process.cc ad784e759a74 > src/arch/arm/registers.hh ad784e759a74 > src/arch/arm/table_walker.hh PRE-CREATION > src/arch/arm/table_walker.cc PRE-CREATION > src/arch/arm/tlb.hh ad784e759a74 > src/arch/arm/tlb.cc ad784e759a74 > src/arch/arm/types.hh ad784e759a74 > src/arch/arm/utility.hh ad784e759a74 > src/arch/arm/utility.cc ad784e759a74 > src/arch/isa_parser.py ad784e759a74 > src/base/loader/elf_object.cc ad784e759a74 > src/base/loader/object_file.hh ad784e759a74 > src/cpu/BaseCPU.py ad784e759a74 > src/cpu/exetrace.cc ad784e759a74 > src/cpu/simple/base.cc ad784e759a74 > src/cpu/simple_thread.hh ad784e759a74 > src/dev/arm/SConscript ad784e759a74 > src/dev/arm/Versatile.py ad784e759a74 > src/dev/arm/versatile.hh ad784e759a74 > src/dev/arm/versatile.cc ad784e759a74 > src/dev/copy_engine.cc ad784e759a74 > src/dev/io_device.hh ad784e759a74 > src/dev/io_device.cc ad784e759a74 > src/sim/process.cc ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/config.ini ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/simerr ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/simout ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/stats.txt ad784e759a74 > util/statetrace/arch/tracechild_arm.hh ad784e759a74 > util/statetrace/arch/tracechild_arm.cc ad784e759a74 > util/statetrace/statetrace.cc ad784e759a74 > > Diff: http://reviews.m5sim.org/r/20/diff > > > Testing > ------- > > > Thanks, > > Ali > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
