THIS IS AN AUTOMATED MESSAGE, DO NOT REPLY. A new Flyspray task has been opened. Details are below.
User who did this: - Ali Saidi (saidi) Attached to Project - M5 Bugs Summary - Bitfield definitions in ARM ISA Task Type - Bug Category - ISA Support Status - New Assigned To - Gabe Black Operating System - All Severity - Medium Priority - Normal Reported Version - 2.0beta5 Due in Version - 2.1 Due Date - 2010-07-31 Details - Steve Reinhardt: 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. Gabe Black wrote: 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: I'm still not very happy with all the redundant bitfield stuff, even if it is caused by IntRegIndex casting issues. (It looks like the big problem is that IntRegIndex is an enum rather than an integer type... is there a big benefit from that? Clearly the cost is pretty high, in my opinion. Why not replace it with a typedef to an integer type and a bunch of constants, maybe in their own Arm::IntReg namespace if that helps?) I'd also like to see the extended bits() notation (or something similar) that concatenates bitfields, e.g., replacing expressions like: (bits(data, 15) << 2) | bits(data,11,10) with: bits(data, 15, 15, 11, 10) (or something similar... I'm open to ideas for better syntax). This makes it more obvious that the bitfields are being concatenated and avoids the magic '2' that really represents the width of bits(_, 11, 10) without being obvious about that. There are even cases where another shift gets folded in that are more confusing; for example, I think this: (bits(machInst, 9) << 6) | (bits(machInst, 7, 3) << 1) is much less clear than: bits(machInst, 9, 9, 7, 3) << 1 More information can be found at the following URL: http://www.m5sim.org/flyspray/task/328 You are receiving this message because you have requested it from the Flyspray bugtracking system. You can be removed from future notifications by visiting the URL shown above. _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
