> 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. > > Gabe Black wrote: > 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?
Maybe you're right... but I still like the idea of using the custom language to make the description more concise, and there does seem to be a lot of redundancy in the C++. Anyway, it's just an aesthetic issue at this point. As far as adding new patches vs fixing old ones: the latter is preferable if it's feasible, but if it's not, then the former is OK. I've found that pushing all your patches then doing a log on a specific file is a good way to see which patches touch that file. > 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. 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... > 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? 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). > 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 > > Gabe Black wrote: > Same as above, plus this is so short. You'd almost have as much text > declaring it as just defining it in place. My opinion is that anything more than a single line is long enough that it should only be inlined if it's necessary for performance. > 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? > > > > Gabe Black wrote: > 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. I'm not sure if gcc at high optimization levels will attempt inlines functions that aren't declared inline or not. My opinion is that if we don't think it should be inlined then it shouldn't be in a header file anyway, so it seems odd to have a function in a header that's not declared inline. The other good thing about inline is that even if the compiler doesn't inline it, you still get the right behavior; if this function was neither static nor inline and it got included in multiple places then you'd have a multiply defined symbol error at link time. Whether to keep the 'static' or not is another question... it should work either way, I think, but I don't know if there's a good reason to leave it there. The only thing I got out of the gcc docs is that a 'static inline' that's not referenced indirectly won't have a stand-alone version generated, on the assumption that no other module could reference it, while a plain 'inline' will. > 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. 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. > 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. 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). > 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. 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. > 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. > > Gabe Black wrote: > Ali's. I'm guessing this is its real name, judging from the instruction > bitfield names. Yea, I assumed the N was meaningful if you were looking at the ARM docs, but it sure isn't when you're not :-). > 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? > > Gabe Black wrote: > That function and the ones above and below it, think. The other ones are > so short it doesn't seem worth the effort. Yea, I don't mind the one-liners in the headers (though my preference for those is to put the whole function on one line if possible). > 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? > > > > Gabe Black wrote: > 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. There's nothing like it in alpha/utility.hh. I say we get rid of it, and if somebody really wants it we should add "%b" to cprintf. > 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. 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. - Steve ----------------------------------------------------------- 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
