> On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > Overall I'm kinda disappointed that most of the decode ended up in C++ and > > not in the ISA description language (I know the latter would have required > > some extensions)... now that it's in place and (presumably) working though, > > I guess there's no compelling reason to go back and do it that way.
I think it actually works out nicely this way. In ISAs like Alpha where the decoder can just sort of fall out of the order you define the instructions, putting it in C++ would be redundant. In ARM, I think the more we enabled it in the parser the more we'd be reimplementing a subset of C++ anyway. Generally, I try to shift things out of the ISA definitions as much as is practical. It does a good job (for instance when building instruction classes) , but in cases like this where it doesn't really contribute much it just adds another layer of complexity to figure out and prevents non-experts (pretty much the world minus maybe 3 or 4 of us) from understanding what's going on. Also, I'm not sure exactly how I'm going to address your comments. Going back and modifying the patches (especially where things need to move between files) would be a big mess, so I'm thinking of adding patches to the end. Thoughts? > 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. > > 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. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/branch.cc, line 34 > > <http://reviews.m5sim.org/r/20/diff/1/?file=178#file178line34> > > > > Is this file really empty now? If so, shouldn't we just get rid of it? Yes it is. We can, but ideally we'd put some disassembly functions in there. I think I just hadn't gotten around to it and then forgot I hadn't. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/macromem.hh, line 112 > > <http://reviews.m5sim.org/r/20/diff/1/?file=179#file179line112> > > > > Be nice not to have deleted the comments describing these classes... or > > at least write new ones. Those were removed because the first half just repeated the class name and the second half was, in the FP instances, basically wrong I think since we now support a more up to date version of the FP instructions. It would be pretty reasonable to add a comment to the top of the whole file explaing the sort of instructions these are for. > 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 Wouldn't that prevent it from being inlined? Or do we not care since it's disassembly? > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/mem.hh, line 454 > > <http://reviews.m5sim.org/r/20/diff/1/?file=181#file181line454> > > > > this should go in a .cc file too Same as above, plus this is so short. You'd almost have as much text declaring it as just defining it in place. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/static_inst.hh, line 63 > > <http://reviews.m5sim.org/r/20/diff/1/?file=188#file188line63> > > > > don't you want these (incl. many more below) declared inline? > > Sure. I think one way or the other we're just asking the compiler pretty please, right? Or does it end up in the object file differently? I'm asking just for my own information and have no problem changing it. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/vfp.hh, line 48 > > <http://reviews.m5sim.org/r/20/diff/1/?file=190#file190line48> > > > > Some comments would be nice... I assume vfp is "vector floating point", > > but it would be good to say that explicitly. That's one of those acronyms that would be well known to anyone familiar with ARM (like SSE, for instance), but generally you're right. This is one of the most recent things I worked on, and it took some effort to get all the exception bits, rounding modes, etc. to cooperate. There are defects in the standard library, deficiencies in gcc as far as code reordering, and differences between the actual hardware in ARM and X86 that made it especially messy. I implemented things one way to start with not knowing what would be required, and then I went back and reorganized things which generally helped a lot. Not everything fit in the reorganization or was actually converted yet, so there's unfortunately two versions of a lot of things. Once it's in a state fit for documenting then it'll be important to document it. I expect it'll end up that way as I work on Neon which will leverage a lot of this. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/vfp.hh, line 245 > > <http://reviews.m5sim.org/r/20/diff/1/?file=190#file190line245> > > > > does this need to be inline? seems like a lot of code (and the ones > > below are worse). Nope. > 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? > > 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. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/vfp.hh, line 1226 > > <http://reviews.m5sim.org/r/20/diff/1/?file=190#file190line1226> > > > > zoinks, a 1200-line header? Yep. At least some of it could be moved to a .cc. A lot of it grew up in place which is why it's in there. There are also a lot of little bits of things that wouldn't make sense to move like the fpAddS function and friends. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/interrupts.hh, line 135 > > <http://reviews.m5sim.org/r/20/diff/1/?file=192#file192line135> > > > > I guess this is subjective, but effectively coding a boolean expression > > as control flow seems weird to me unless it's *really* complicated > > otherwise. So I'd write this as: > > return ((interrupts[INT_IRQ] && !cpsr.i) || > > (interrupts[INT_FIQ] && !cpsr.f) || > > (interrupts[INT_ABT] && !cpsr.a) || > > interrupts[INT_RST]); > > Ali did most of the interrupt stuff so he'd have to comment here, but your suggestion seems reasonable to me. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/isa.hh, line 93 > > <http://reviews.m5sim.org/r/20/diff/1/?file=195#file195line93> > > > > This seems like a really big and rarely used function to be defined in > > the .hh file. Yes. It didn't start life so big, and it grew slowly. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/isa.hh, line 115 > > <http://reviews.m5sim.org/r/20/diff/1/?file=195#file195line115> > > > > I don't get this comment... it sounds like you're explaining why cpacr > > isn't 0 even though it should be, but then you set it to 0 anyway. I think that comment is stale which makes it confusing but it's still technically correct. cpacr controls whether coprocessors (including the fp ones) are accessible (and with what privilege? I forget the details) and we only support having everything turned on at the moment. The reset value for this register should be 0, I think, but then we would panic if someone tried to turn anything off. Later that turned out to be a problem for other reasons, so I turned the panic into a warning (I think) and then made this 0 like it was supposed to be. When the comment says "Technically this should be zero" it's right but redundant, and when it says "but we don't support those settings" it describes the real, current issue. One way or the other, though, it needs to be updated. > 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? 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 la st I tried. > 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) 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. > 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)? 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. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/pagetable.hh, line 199 > > <http://reviews.m5sim.org/r/20/diff/1/?file=250#file250line199> > > > > 'inline' is redundant here and on next method This one is Ali's again, and I agree here too. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/predecoder.hh, line 110 > > <http://reviews.m5sim.org/r/20/diff/1/?file=252#file252line110> > > > > Can we move all this code to a .cc file? That function and the ones above and below it, think. The other ones are so short it doesn't seem worth the effort. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/table_walker.hh, line 81 > > <http://reviews.m5sim.org/r/20/diff/1/?file=256#file256line81> > > > > Should have a blank line between these function defs Ali's. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/table_walker.hh, line 174 > > <http://reviews.m5sim.org/r/20/diff/1/?file=256#file256line174> > > > > What is N? Unless this is obvious to anyone who knows anything about > > ARM page tables (a set that doesn't include me) a longer name would be > > nice. In any case, a comment is called for. Ali's. I'm guessing this is its real name, judging from the instruction bitfield names. > On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/utility.hh, line 149 > > <http://reviews.m5sim.org/r/20/diff/1/?file=261#file261line149> > > > > What's the point of 'static' here (and on the previous function)? > > > > Also, this seems like a more generic "print binary" function... can we > > just add "%b" to cprintf? > > Isn't that required when you have a function in a header file? One way or the other it was that way when I got here :). I think really we could get rid of this entirely. I haven't done that since I didn't have a need to, but I'd need to transcribe the bits onto paper (or notepad-esque software) to split out the bitfields anyway. I might as well do that with the shorter hex and then expand it in place with handy every-four-bits delineation already in place. I think this was inherited from Alpha maybe? But in any case even if it isn't strictly necessary %b seems pretty harmless. - 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
