ARM: Implement majority of instructions and some initial full-system support

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

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.

Testing Done

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 May 8, 2010, 8:50 a.m. (May 8, 2010, 8:50 a.m.)

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?

Steve Reinhardt May 10, 2010, 3:03 a.m. (May 10, 2010, 3:03 a.m.)

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.

src/arch/arm/faults.hh (Diff revision 1)

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 May 8, 2010, 7:35 a.m. (May 8, 2010, 7:35 a.m.)

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 May 8, 2010, 2:06 p.m. (May 8, 2010, 2:06 p.m.)

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...

Gabe Black May 10, 2010, 5:01 p.m. (May 10, 2010, 5:01 p.m.)

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.

Gabe Black May 22, 2010, 5:40 p.m. (May 22, 2010, 5:40 p.m.)

On closer inspection I don't think I want to change these names. Like you pointed out there's already a Fault typedef so renaming ArmFault Fault would cause a lot of confusion. A similar but easier to tolerate problem is that renaming the ArmFaultVals template class would making it collide with the structure defined in ArmFault. It should be clear to the compiler what's going on, I think, but it'd be very confusing for a person to keep track of which FaultVals you're talking about, especially when crtp is involved.

We could potentially rename the Fault typedef like you seem to be suggesting, but we'd have to change quite a bit of code. I'm not convinced it would be worth it since it's not clear what other name would be much better.

src/arch/arm/insts/branch.cc (Diff revision 1)

Is this file really empty now?  If so, shouldn't we just get rid of it?

Gabe Black May 8, 2010, 7:43 a.m. (May 8, 2010, 7:43 a.m.)

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.

src/arch/arm/insts/macromem.hh (Diff revision 1)

Be nice not to have deleted the comments describing these classes... or at least write new ones.

Gabe Black May 8, 2010, 7:46 a.m. (May 8, 2010, 7:46 a.m.)

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.

src/arch/arm/insts/mem.hh (Diff revision 1)

seems like this should go in a .cc file

Gabe Black May 8, 2010, 7:47 a.m. (May 8, 2010, 7:47 a.m.)

Wouldn't that prevent it from being inlined? Or do we not care since it's disassembly?

Steve Reinhardt May 8, 2010, 2:07 p.m. (May 8, 2010, 2:07 p.m.)

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).

Gabe Black May 10, 2010, 5:08 p.m. (May 10, 2010, 5:08 p.m.)

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.

Steve Reinhardt May 11, 2010, 1:17 a.m. (May 11, 2010, 1:17 a.m.)

That's not a bug, that's a feature :-).  Seriously, it's always been the case that StaticInst objects are reused for identical instructions appearing at different PCs.  I don't think we've ever done any measurement on it, but my recollection from other projects is that this is a pretty significant memory savings over creating a new isntance for every PC.  Expecting that the PC value would be constant reflects not understanding how it's supposed to work.  Is PC-relative addressing the issue?  If it's really that important to have those values be printed as absolute addresses, we could extend the disassembly to incorporate PC-relative values in such a way that they do get recalculated.

Gabe Black May 11, 2010, 8:10 a.m. (May 11, 2010, 8:10 a.m.)

The fix was specifically for PC relative branches that print their target along with a symbol+offset, although the problem is more general than that. Those branches are pretty common, I think, and I would guess are also used in the Alpha stuff.

Note that I'm not advocating making the decode cache on hit on the same inst at the same PC, although I'm also not arguing the opposite since I don't know one way or the other. I'm just saying our disassembled string may be stale and it would be nice to fix that.

Steve Reinhardt May 11, 2010, 9:55 a.m. (May 11, 2010, 9:55 a.m.)

Sometimes it helps to actually look at the code... there is a documented way to deal with this:
http://repo.m5sim.org/m5/file/24379f92cc10/src/cpu/static_inst.hh#l423

We can discuss whether that documented method could be improved on, but there's no "glitch" to be worked around.  Not only that, but this code is so old it predates our revision control (it came in on changeset 2)... I think that means it pre-dates our switch to bitkeeper.

Gabe Black May 11, 2010, 4:22 p.m. (May 11, 2010, 4:22 p.m.)

Yep, you're right. I didn't realize that function was virtual and I didn't look into it too deeply. I saw the Florida guys attempt to fix things, looked at the code a little bit, and made a mental note to do something about it later, later happening to be now.

src/arch/arm/insts/mem.hh (Diff revision 1)

this should go in a .cc file too

Gabe Black May 8, 2010, 7:48 a.m. (May 8, 2010, 7:48 a.m.)

Same as above, plus this is so short. You'd almost have as much text declaring it as just defining it in place.

Steve Reinhardt May 8, 2010, 2:08 p.m. (May 8, 2010, 2:08 p.m.)

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.

src/arch/arm/insts/static_inst.hh (Diff revision 1)

don't you want these (incl. many more below) declared inline?

Gabe Black May 8, 2010, 7:51 a.m. (May 8, 2010, 7:51 a.m.)

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.

Steve Reinhardt May 8, 2010, 2:14 p.m. (May 8, 2010, 2:14 p.m.)

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.

src/arch/arm/insts/vfp.hh (Diff revision 1)

Some comments would be nice... I assume vfp is "vector floating point", but it would be good to say that explicitly.

Gabe Black May 8, 2010, 7:57 a.m. (May 8, 2010, 7:57 a.m.)

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.

src/arch/arm/insts/vfp.hh (Diff revision 1)

does this need to be inline?  seems like a lot of code (and the ones below are worse).

Gabe Black May 8, 2010, 7:58 a.m. (May 8, 2010, 7:58 a.m.)
```
Nope.
```

src/arch/arm/insts/vfp.hh (Diff revision 1)

What's the purpose of these __asm__ statements?

Gabe Black May 8, 2010, 8:02 a.m. (May 8, 2010, 8:02 a.m.)

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 May 8, 2010, 2:47 p.m. (May 8, 2010, 2:47 p.m.)

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.

Gabe Black May 10, 2010, 5:20 p.m. (May 10, 2010, 5:20 p.m.)

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.

Steve Reinhardt May 11, 2010, 1:19 a.m. (May 11, 2010, 1:19 a.m.)

Hmm, I'd be surprised there's no better solution.  Certainly this is the kind of thing that "volatile" is supposed to fix in a standardized fashion.  Anybody else have an opinion on this?  Nate?

Ali Saidi May 11, 2010, 6:02 a.m. (May 11, 2010, 6:02 a.m.)

We went through several revisions before coming up with this one. I imagine that feXXXXXX() should actually be marked in the standard library in such a way that disallows code motion around it, but it doesn't appear to work. I think the best solution would be to add something a define in base/compiler.hh called M5_CODE_BARRIER and define it to the asm at the moment. I don't like the volatile in this use case since it prevents many compiler optimizations for the code.

Steve Reinhardt May 11, 2010, 6:51 a.m. (May 11, 2010, 6:51 a.m.)

I'm surprised that copying op1 and op2 to local volatile vars before calling fesetround() and then doing the operation on those temp vars wouldn't fix it.  Seems like that would be a gcc bug if it didn't work.

Gabe Black May 11, 2010, 8:11 a.m. (May 11, 2010, 8:11 a.m.)

I thought of putting them in a macro, but I think an important part of the voodoo is that they refer to the variables in question as inputs and outputs. I'm not sure exactly how that would get wrapped into a macro. A generic code barrier would be just as effective, though, if we can find a nice way to build one.

This is in some ways a gcc deficiency since there's supposed to be a way around this problem generally. It's not implemented, though, or something like that. I forget the details.

Steve Reinhardt May 11, 2010, 9:45 a.m. (May 11, 2010, 9:45 a.m.)

Yea, like I said, it seems to me that if using a set of volatile local vars in an appropriate way doesn't fix it, that'd be a gcc bug.

src/arch/arm/insts/vfp.hh (Diff revision 1)

zoinks, a 1200-line header?

Gabe Black May 8, 2010, 8:09 a.m. (May 8, 2010, 8:09 a.m.)

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.

src/arch/arm/interrupts.hh (Diff revision 1)

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]);

Gabe Black May 8, 2010, 8:10 a.m. (May 8, 2010, 8:10 a.m.)

Ali did most of the interrupt stuff so he'd have to comment here, but your suggestion seems reasonable to me.

Ali Saidi May 11, 2010, 6:05 a.m. (May 11, 2010, 6:05 a.m.)
```
I'll change this.
```

src/arch/arm/isa.hh (Diff revision 1)

This seems like a really big and rarely used function to be defined in the .hh file.

Gabe Black May 8, 2010, 8:11 a.m. (May 8, 2010, 8:11 a.m.)
```
Yes. It didn't start life so big, and it grew slowly.
```

src/arch/arm/isa.hh (Diff revision 1)

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.

Gabe Black May 8, 2010, 8:16 a.m. (May 8, 2010, 8:16 a.m.)

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.

src/arch/arm/isa/bitfields.isa (Diff revision 1)

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 May 8, 2010, 8:30 a.m. (May 8, 2010, 8:30 a.m.)

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 May 10, 2010, 2:46 a.m. (May 10, 2010, 2:46 a.m.)

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).

Gabe Black May 10, 2010, 5:56 p.m. (May 10, 2010, 5:56 p.m.)

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.

Steve Reinhardt May 11, 2010, 1:26 a.m. (May 11, 2010, 1:26 a.m.)

What about getting rid of def bitfield entirely and only using defined fields on ExtMachInst?  I agree that the macro stuff would be nice to get rid of; it looks cute, but it's not very robust.  And if they're not macros anymore, maybe you won't object to them being lowercase?

Gabe Black May 11, 2010, 8:03 a.m. (May 11, 2010, 8:03 a.m.)

That sounds good to me, and I wouldn't mind if they weren't macros and the C++ had to stick machInst in there. If the extra text got too annoying the StaticInsts could have inline accessors that pulled from machInst for you. I'd still be a lot more comfortable with that if we fixed the BitUnion bug. We should do that in general since it does come up from time to time. Ali ran into it with this Arm stuff, as a matter of fact.

src/arch/arm/isa/decoder/decoder.isa (Diff revision 1)

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 May 8, 2010, 8:35 a.m. (May 8, 2010, 8:35 a.m.)

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 May 10, 2010, 2:51 a.m. (May 10, 2010, 2:51 a.m.)

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.

Gabe Black May 10, 2010, 5:43 p.m. (May 10, 2010, 5:43 p.m.)

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.

Steve Reinhardt May 11, 2010, 1:23 a.m. (May 11, 2010, 1:23 a.m.)

Yea, we should probably either drop the '^' in the regex or add a '\n' where we insert it, and see if that fixes the problem.

src/arch/arm/isa/formats/data.isa (Diff revision 1)

Is there a reason we can't use predefined bitfields here (and a ton of other places)?

Gabe Black May 8, 2010, 8:40 a.m. (May 8, 2010, 8:40 a.m.)

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 May 10, 2010, 3:02 a.m. (May 10, 2010, 3:02 a.m.)

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.

Gabe Black May 10, 2010, 5:42 p.m. (May 10, 2010, 5:42 p.m.)

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.

Steve Reinhardt May 11, 2010, 1:45 a.m. (May 11, 2010, 1:45 a.m.)

I do see lots of repeated definitions of rn, rd, and rm.  Also for other fields like lsb, msb, rotation, that are defined in multiple places (e.g., 2 out of 3 branches of an if/else if tree) I'd prefer to see them defined at the top in a common place; I don't think that avoiding extracting them in the one case where they're not needed is a real performance issue.  If our decode cache isn't broken then actual from-scratch decode should not be a performance issue at all.

I had a couple of other ideas:
1. Maybe we should define bits() as a method on ExtMachInst, so we can say machInst.bits(X,Y).  That's not a huge win by itself, but if we couple it with getting rid of "def bitfield" and have bitfields in the ISA language be attributes on the ExtMachInst, then you could even say "decode bits(5,3)" right in the ISA language.
2. We could extend bits() to take multiple pairs of bit indices, concatenating the indicated ranges, so expressions like:
    timm = (bits(machInst, 19, 16) << 12) | bits(machInst, 11, 0);
could be rewritten as:
    timm = machInst.bits(19, 16, 11, 0);

I think #2 would even be useful in converting some of your if/else if structures back to switches, which I find preferable because they make it clear(er) that you've covered all the possible cases.

Ali Saidi May 11, 2010, 6:30 a.m. (May 11, 2010, 6:30 a.m.)

I think that this would certainly clear up a few things.

Gabe Black May 11, 2010, 8 a.m. (May 11, 2010, 8 a.m.)

I've had thoughts similar to these. For instance, it would be nice to add a function to either the ExtMachInst (or the StaticInst) that would return an IntRegIndex from a set of bits. That would get rid of some of the stacks of casts that tend to happen around register indexes. It wouldn't be that bad (knock on wood) to add new types of bitfields to BitUnions that would do arbitrarily complex things when reading or writing bits. The problem is that they'd need to be set up first, and if they only show up once or twice the overhead makes them a lot more verbose than just calling bits directly.

I'd be fine with moving the common definitions to the tops of the blocks they're used in. The thing to watch out for is bitfields that are -almost- the same but that are changed around sometimes. There are instances of that and they have caused some bugs.

While these sorts of extensions are a good idea and would clean things up even outside of ARM in some places, they won't remove all instances of having to do things the hard way because there are plenty of places where the bits of interest shift for what seems like (or literaly is) every different instruction in a group, or where things are combined like bit a == bit b, or a == 1 and b != 010. It's sort of a mess.

Steve Reinhardt May 11, 2010, 9:44 a.m. (May 11, 2010, 9:44 a.m.)

Are the IntRegIndex casts really necessary?  I'm surprised you can't get by on an implicit conversion.

I saw some of those weird cases... I think some (maybe many) of them could be cleaned up, but I would not be surprised if there are a few that resist normalization.

src/arch/arm/pagetable.hh (Diff revision 1)

'inline' is redundant here and on next method

Gabe Black May 8, 2010, 8:40 a.m. (May 8, 2010, 8:40 a.m.)
```
This one is Ali's again, and I agree here too.
```
Ali Saidi May 11, 2010, 6:31 a.m. (May 11, 2010, 6:31 a.m.)
```
Will remove.
```

src/arch/arm/predecoder.hh (Diff revision 1)

Can we move all this code to a .cc file?

Gabe Black May 8, 2010, 8:43 a.m. (May 8, 2010, 8:43 a.m.)

That function and the ones above and below it, think. The other ones are so short it doesn't seem worth the effort.

Steve Reinhardt May 10, 2010, 2:55 a.m. (May 10, 2010, 2:55 a.m.)

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).

src/arch/arm/table_walker.hh (Diff revision 1)

Should have a blank line between these function defs

Gabe Black May 8, 2010, 8:43 a.m. (May 8, 2010, 8:43 a.m.)
```
Ali's.
```
Ali Saidi May 11, 2010, 6:31 a.m. (May 11, 2010, 6:31 a.m.)
```
Ok.
```

src/arch/arm/table_walker.hh (Diff revision 1)

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 May 8, 2010, 8:44 a.m. (May 8, 2010, 8:44 a.m.)

Ali's. I'm guessing this is its real name, judging from the instruction bitfield names.

Steve Reinhardt May 10, 2010, 2:55 a.m. (May 10, 2010, 2:55 a.m.)

Yea, I assumed the N was meaningful if you were looking at the ARM docs, but it sure isn't when you're not :-).

Ali Saidi May 11, 2010, 6:33 a.m. (May 11, 2010, 6:33 a.m.)

N is the number of bits that a page represents. I'll add doxygen comments to the fields.

src/arch/arm/utility.hh (Diff revision 1)

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 May 8, 2010, 8:49 a.m. (May 8, 2010, 8:49 a.m.)

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.

Steve Reinhardt May 10, 2010, 2:58 a.m. (May 10, 2010, 2:58 a.m.)

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.

src/arch/arm/isa/formats/data.isa (Diff revision 1)

It looks like cases 1 & 3 (and 9 and 0xb) are identical; right?  If so, I think they should be merged; getting rid of redundant code trumps keeping the cases in numerical order.

Gabe Black May 11, 2010, 8:14 a.m. (May 11, 2010, 8:14 a.m.)

I'll have to look at the manual this evening (hopefully) and check, but it may be certain cases are supposed to fall through to other cases. Even though their code is the same, one instance would fall through to further checks and the other would just UD. I'm not saying that's the case here, but it did come up sometimes. Looking at the code it seems I'm letting it fall through in one case but have a default: label in the other. That may be a bug.

src/arch/arm/isa/formats/data.isa (Diff revision 1)

In these cases they're all "return" statements, so there's no fall-through issue here.

Change Summary:

Address most of the feedback

Diff:

Revision 2 (+20123 -2895)

Show changes

	configs/common/cpu2000.py
	src/arch/isa_parser.py
	src/arch/arm/ArmTLB.py
	src/arch/arm/SConscript
	src/arch/arm/faults.hh
	src/arch/arm/faults.cc
	src/arch/arm/interrupts.hh
	src/arch/arm/interrupts.cc
	107 more

src/arch/arm/faults.cc (Diff revision 2)

I know that I'm being difficult here, but can you make the comment not be on the /* line, but on the following line?  I have lots of copyright scripts with regular expressions in them and this would screw them up.

src/arch/arm/insts/mem.cc (Diff revision 2)

This is pretty minor, but I think this code is both more compact and expresses the intent better (for this case and similarly the following ASR case):
ccprintf(os, " LSR #%d", (shiftAmt == 0) ? 32 : shiftAmt);

Ali Saidi June 1, 2010, 6:04 a.m. (June 1, 2010, 6:04 a.m.)
```
Fixed
```

My comments are very superficial, but I would appreciate the copyright fixes to make life easier if we ever have to do some copyright nonsense again (I sure hope not).  I have one other minor request.  Get rid of lines > 80 columns for at least the ARM stuff.

configs/common/cpu2000.py (Diff revision 2)

No big deal, but python doesn't require parens for stuff like this.

Ali Saidi June 1, 2010, 6:04 a.m. (June 1, 2010, 6:04 a.m.)

Yup, I know, I was just matching the style of the if statements below it

src/arch/arm/SConscript (Diff revision 2)

Unless you know that none of this was written by FSU, you probably shouldn't remove the copyright.

Ali Saidi June 1, 2010, 4:55 a.m. (June 1, 2010, 4:55 a.m.)
```
It was moved below the arm specific blob
```

src/arch/arm/faults.hh (Diff revision 2)

You're going to fix this for me, right?

Ali Saidi June 1, 2010, 5:57 a.m. (June 1, 2010, 5:57 a.m.)
```
These are all fixed
```

src/arch/arm/faults.hh (Diff revision 2)

Why are you specifying the values in this enum?  Are they important?  Do they come from some document?  A comment might be nice to explain why.  If the values don't matter, then they should probably be removed.

Ali Saidi June 1, 2010, 6:03 a.m. (June 1, 2010, 6:03 a.m.)
```
Added a comment
```

src/arch/arm/insts/macromem.hh (Diff revision 2)

Fix please

src/arch/arm/insts/macromem.cc (Diff revision 2)

Fix please

src/arch/arm/insts/mult.hh (Diff revision 2)

Fix please

src/arch/arm/insts/pred_inst.hh (Diff revision 2)

Fix please

src/arch/arm/insts/pred_inst.cc (Diff revision 2)

Fix please

src/arch/arm/insts/static_inst.hh (Diff revision 2)

Fix please

src/arch/arm/insts/static_inst.cc (Diff revision 2)

Fix please

src/arch/arm/insts/vfp.cc (Diff revision 2)

fix please

src/arch/arm/isa.hh (Diff revision 2)

Another fix here please.

src/arch/arm/isa_traits.hh (Diff revision 2)

This get resolved?

Ali Saidi June 1, 2010, 6:03 a.m. (June 1, 2010, 6:03 a.m.)
```
it will remain for now
```

src/arch/arm/miscregs.hh (Diff revision 2)

Another fix.

src/arch/arm/miscregs.cc (Diff revision 2)

How often does this happen?  Is there nothing more efficient than this insane implementation?  A table lookup perhaps?

Ali Saidi June 1, 2010, 5:57 a.m. (June 1, 2010, 5:57 a.m.)
```
Not often
```

Gabe Black June 1, 2010, 5:52 p.m. (June 1, 2010, 5:52 p.m.)

Yeah, this isn't often. ARM uses coprocessors for a few important things. Two are for floating point, one for single precision and one for double I believe, and coprocessor 15 has a bunch of control registers in it. There are four dimensions to the control register decode, two indexes and two opcodes, and the space for the control registers is irregularly and incompletely filled. The documentation shows it as a tree, basically, and this implementation mirrors that. The function looks really complicated, but it should be decently efficient because it's nested to a fixed depth and usually uses switch statements unless there's only one or two choices. I don't think a lookup table would work very well given how the space is structured. We'd either have to have a huge table, or we'd end up with something as complicated as this but who's implementation was less obvious.

src/arch/arm/predecoder.hh (Diff revision 2)

Fix please.

src/arch/arm/predecoder.cc (Diff revision 2)

Fix please

src/arch/arm/process.cc (Diff revision 2)

Spacing looks messed up at least in reviewboard

Ali Saidi June 1, 2010, 6:03 a.m. (June 1, 2010, 6:03 a.m.)
```
Fixed
```

src/arch/arm/types.hh (Diff revision 2)

Fix please

src/arch/arm/utility.hh (Diff revision 2)

Fix please

src/cpu/simple/base.cc (Diff revision 2)

This change really require copyright?

src/dev/arm/Versatile.py (Diff revision 2)

Please remove ##

Ali Saidi June 1, 2010, 6:03 a.m. (June 1, 2010, 6:03 a.m.)
```
Fixed
```

src/dev/arm/versatile.hh (Diff revision 2)

Please fix

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Change Summary:

Diff: