I don't see any responses from you on reviewboard, Gabe; clicking on "View Reviews" does show a review from you at "October 24th, 2010, 12:30 a.m.", but it's empty as far as I can tell.
Anyway, as I think I mentioned earlier, I'm thinking that email works better for back-and-forth conversations than trying to go through reviewboard, so that's fine with me. On Sun, Oct 24, 2010 at 12:31 AM, Gabe Black <[email protected]> wrote: >> What's the distinction between calling instAddr() and pc() (and similarly >> nextInstAddr() vs npc())? It looks to me like they're currently >> interchangeable, yet sometimes one is used and sometimes the other. >> > > instAddr, nextInstAddr, and microPC are guaranteed to be there, read > only, and what ISA independent code is allowed to use. pc, npc, etc. are > read/write accessors, there at the whim of the ISA and for its exclusive > use in managing the PC. Another distinction which came up in our recent > discussion of the PAL bit is that instAddr and nextInstAddr are supposed > to be literal memory addresses for the instruction, hence the name > change. The "pc", which is usually equivalent but not always like in the > case of Alpha, is an ISA level concept. Part of what might be confusing > is that a lot of the time they're equal to each other. Another somewhat > contrived example would be if we were to implement an ISA with a word > addressed PC like the LC2K ISA (all the people not from MI now say "the > what?"). In an effort towards consistency and towards sharing the > implementation among ISAs, the pc, npc, etc. scheme is used throughout > as applicable. OK, that seems like a good explanation, but (1) as Ali said, it really needs to go in a doxygen comment where these accessors are defined in the base PCState class and (2) I seem to recall some specific comments where substitutions were made that didn't necessarily make sense wrt this comment, so it would be good to double-check those, and possibly add some explanatory comments to the less obvious uses as well. >>> The PCs were previously managed entirely by the CPU which had to know about >>> PC >>> semantics, try to figure out which dimension to increment the PC in, what to >>> set NPC/NNPC, etc. These decisions are best left to the ISA in conjunction >>> with the PC type itself. Because most of the information about how to >>> increment the PC (mainly what type of instruction it refers to) is contained >>> in the instruction object, a new advancePC virtual function was added to the >>> StaticInst class. Subclasses provide an implementation that moves around the >>> right element of the PC with a minimal amount of decision making. In ISAs >>> like >>> Alpha, the instructions always simply assign NPC to PC without having to >>> worry >>> about micropcs, nnpcs, etc. The added cost of a virtual function call should >>> be outweighed by not having to figure out as much about what to do with the >>> PCs and mucking around with the extra elements. >>> >> >> Does it make sense to go through an ISA-specific function, for example: >> >> inline >> TheISA::advancePC(PCState &pcState, StaticInstPtr inst) >> { >> pcState.advance(); >> OR >> inst->advancePC(pcState); >> } >> > > As far as pcState.advance(), no. The idea is that the instruction knows > what would follow it (a microop or a new instruction) and advances the > PC in the right dimension. As far as inst->advancePC that would > functionally work, but I'm not sure how it would be useful. Wouldn't > that just obscure what's going on? I'm not sure we're communicating here... you're talking about how in some ISAs (like x86) you need to know the current instruction to know how to advance the PC, and in other ISAs (like Alpha) you don't, and always calling inst->advancePC(pcState) costs you a virtual function call even in the latter case. My suggestion is that this ISA-specific advancePC function (where the "OR" is a compile-time decision based on which ISA you're using) would allow you to avoid the virtual function call when it's not needed. >>> ISA parser: >>> >>> To support the new API, all PC related operand types were removed from the >>> parser and replaced with a PCState type. There are two warts on this >>> implementation. First, as with all the other operand types, the PCState >>> still >>> has to have a valid operand type even though it doesn't use it. Second, >>> using >>> syntax like PCS.npc(target) doesn't work for two reasons, this looks like >>> the >>> syntax for operand type overriding, and the parser can't figure out if >>> you're >>> reading or writing. Instructions that use the PCS operand (which I've >>> consistently called it) need to first read it into a local variable, >>> manipulate it, and then write it back out. >>> >> >> See my comments below... I don't see why this change is necessary or even >> desirable as opposed to just making the existing PC/NPC/NNPC/UPC etc. >> operands do the right thing, at least in the common cases. >> > > I would have really like to keep the old syntax since I completely agree > this new version is a bit clunky and round about. The basic problem is > that unlike in the previous system, there aren't accessors for reading > and writing parts of the PC since what those parts are isn't defined > external to the ISA. Because of that, an instruction that wants to > modify the PC needs to bring in a copy of the whole thing, do any > modifications locally, and then send the whole revised version back. > This could be done locally to update each operand, but then you'd have a > lot of wasteful copying of the PCState structure and calls to get/set it. > > One possible solution would be to keep track of whether -any- member of > the PC was being accessed, read it in once, use it, and if any member > was written to write it out at the end. Then the individual components > could be accessed with operands. I think I understand everything you're saying up to here. I think we agree that you could take an ISA description statement like NNPC = foo; and automatically rewrite it to something like PCState _tmp_pc = tc->pc(); _tmp_pc.nnpc(foo); tc->pc(_tmp_pc); if necessary, or perhaps something a little simpler if possible. Your concern seems to be that if we have multiple statements like PC = foo; NPC =bar; then a naive translation would do two read-modify-writes on the pcState. I agree that's a potential issue, but how many times does that really happen? In particular, it seems like there are a lot of places where we used to have two updates like PC = foo; NPC = foo + 4; where we've already replaced it (or could replace it) with a more simple call like setPC(foo). Does it even make sense in most ISAs to update PC without updating NPC? I feel like for both the ISA code and for the regular C++ code that manipulates the PC we should be able to identify the common patterns (like the way that setting the PC typically updates NPC to PC+sizeof(inst)) and make those the default behaviors, and then in 90+% of the cases we are at most only deleting redundant lines like setting NPC rather than actually changing the sequence of how things get set. > This could be a little limitting, > though, because the names, quantities, arguments, etc. of the accessors > and raw members aren't fixed. We'd have to have a syntax for sort of > dynamically building up an operand type on a per ISA basis to give > access to all the bits of the PC. This would all be possible, but I > think is outside the scope of this already very large change. You lost me here... Steve _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
