Sorry for the wait, Tim. Here are my comments. These are sort of stream of consciousness style.
1. You don't necessarily need to credit the previous author for very generic files like Sconscripts. The versions in ARM were doubtlessly copied from another ISA in the first place anyway. For files where there's a significant amount of original, "creative" code from a previous author, you should put both their name and your name as authors, I believe. Ultimately there shouldn't be any code like that because it's best to avoid the duplication, but it's hard to avoid at this stage. 2. It would be great if you could go through and eliminate everything you didn't write for PowerPC or is required to compile, including the body of functions that need to be stubbed out but aren't run. Depending on when you copied over ARM it might have had carry overs from Alpha or MIPS (or both? I forget), so we end up seeing triple (or quadruple?) when looking at that code. It'll still be in the other directory if you need to look at it. You may have already done this since I haven't read through much yet. If so I apologize. 3. In faults.cc you should look into using a base class to hold some of the generic fault entering code. There are parts that look like they change between fault types, but it looks like at least some portions can be factored out. 4. Since I see it in faults.cc I'll mention it here. Please adjust your code to fit our style guidelines. Nate mentioned some things to fix, but there are some others too. There should be spaces after commas, braces aren't always in the right places, there should be spaces between ifs, fors, etc. and the opening parenthesis, commented out lines should be deleted, not commented out, indentation is four spaces, return types go on their own lines, etc. It's a pain and it seems trivial (I complained when I was first subjected to it too) but we need to be consistent. 5. What is printVal in PowerPCStaticInst for? It looks like something that could be replaced with a DPRINTF, or at least ccprintf. 6. It might be slightly better to use the bitfield BF in cmpi and cmpli instead of machInst.bf. The bitfields ultimately end up as #defines, so they're available in the C++ as well. It wouldn't be the end of the world to leave it like it is, though. 7. In divw, etc. make sure to set Rt no matter what, even if you set it equal to itself. The ISA parser doesn't (and arguably shouldn't) try to understand when a variable is or isn't written to, so if you don't set it to anything, the variable will still be updated with uninitialized garbage. This will have the side effect of making Rt a source even in the noop like case, but hardware would arguably behave the same. 8. In cmpb you're setting bits in a copy of Ra and then throwing them away. I'm guessing you might be trying to use the similar function replaceBits which hides updates in place, but you shouldn't use that here. By hiding the assignment, you trick the isa parser into thinking Ra is a source and not a destination. You should use insertBits and an assignment. 9. In stwcx (and potentially others) it looks like you're doing a conditional memory access. There's probably not a much better way to do that right now, but be warned that we (to my knowledge) always assume a memory instruction accesses memory, and may end up stalled waiting for it to do so in some CPU models (O3 at least, I think). 10. PowerPC seems to fit into the ISA parser very nicely! That's great to see. 11. In the FloatDoubleDecode template, rather than panic, it would be better to return an instruction that does nothing but return a bad instruction fault (whatever that is in PowerPC) when executed. Execution may go down totally bogus paths with speculative execution and the decoder may be presented with totally random values. Unless something in M5 has malfunctioned you shouldn't panic. The ISA should have a mechanism for throwing a tantrum if that data is correct path instruction memory and doesn't mean anything. 12. The EAComp and MemAcc classes aren't used by anything as far as I know. Since your version would therefore never be tested and isn't really necessary, it should probably be deleted/factored out. I think you'll find that simplifies things a lot. Steve can comment on the original/future intention for this code. 13. There's a lot of Alpha gunk in isa_traits.hh which can go. If it compiles without it and you don't know what it's for, nuke it. It can be re-added later in with the right PowerPC values. 14. It looks like there may be some tabs in linux/linux.hh, and if I remember correctly you may have inherited those. Please replace them with spaces. 15. linux/process.hh has ARM cruft. 16. In registers.hh, FPControlRegNums looks very suspicious. What are you doing with it? All in all, this is some very nice work. We appreciate you're sharing it with us, and if you can please polish out these rough spots we should hopefully be able to get it into the tree soon (ish). Also, there's also a lot here so I had to skim a bit. If there's anything you can think of that might not be quite right that we missed, please mention it so we can double check. Thanks! Gabe Timothy M Jones wrote: > On Sun, 30 Aug 2009 01:21:22 +0100, Daniel Becker <[email protected]> > wrote: > > >> On Aug 29, 2009, at 5:00 PM, Gabriel Michael Black wrote: >> >> >>> Without looking at the actual code, the PC operand class sounds fine. >>> For when I get a chance to look at your patches in more depth, what >>> documentation (preferably downloadable) for PowerPC would you >>> recommend? >>> >> The most recent version (2.06) of the Power ISA specification is >> available for download at Power.org: >> >> <http://www.power.org/resources/reading/> >> >> > Exactly. That's the version I've been using. > > Cheers > Tim > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
