I won't argue whether this forwarding should be done this way since we already beat that horse to death. I do think all the changes that are being added to the parser here could (and should) be done elsewhere pretty easily. Otherwise this looks like it only needs to be adjusted a little bit and it should be good to go.

src/arch/isa_parser.py (Diff revision 1)

Once the changes below are moved out, this copyright should be removed.

1. Ali Saidi Dec. 9, 2010, 8:10 a.m. (Dec. 9, 2010, 8:10 a.m.)

   Done

src/arch/isa_parser.py (Diff revision 1)

These values aren't passed directly to anything, they're exposed through a header file and then imported into the ArmISA namespace in registers.hh. You could define the max source index to be the value generated here plus the dest value. That gets this change out of the parser and avoids bloating the instruction objects for other ISAs that don't need it.

1. Ali Saidi Dec. 9, 2010, 8:09 a.m. (Dec. 9, 2010, 8:09 a.m.)

   So you mean don't include max_inst_regs.hh and just put the constants in registers.hh?
   
   Ali
   

2. Gabe Black Dec. 9, 2010, 8:17 a.m. (Dec. 9, 2010, 8:17 a.m.)

   No. You'd still include that, you just wouldn't reexport it directly with these lines:
   
   namespace ArmISA {
   
   using ArmISAInst::MaxInstSrcRegs;
   using ArmISAInst::MaxInstDestRegs;
   
   You'd create new constants with the same names but in the ArmISA namespace that were derived from the values in the header file which are in the ArmISAInst namespace.

src/arch/isa_parser.py (Diff revision 1)

I really really don't like ARM specific code in the isa_parser. This should at least be behind a generalized mechanism if not moved out of the parser all together. Why couldn't this code be added to the constructor template after the %(constructor)s line? There doesn't seem to be anything in it that actually requires knowledge from the parser.

1. Ali Saidi Dec. 9, 2010, 8:10 a.m. (Dec. 9, 2010, 8:10 a.m.)

   Done.

src/cpu/o3/iew_impl.hh (Diff revision 1)

Don't add these blank lines.

1. Ali Saidi Dec. 9, 2010, 8:10 a.m. (Dec. 9, 2010, 8:10 a.m.)

   Done.

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

I'm not quite sure this is right. Ignoring the predicate case, the load -has- executed, but its access (or the load itself) faulted. It would be appropriate to have this here but conditioned on the predicate actually being false. That would be like the store case below. And actually, if the instruction faulted, won't all the registers be rolled back anyway?

1. Ali Saidi Dec. 9, 2010, 8:13 a.m. (Dec. 9, 2010, 8:13 a.m.)

   I don't think it hurts anything to have it here, since as you point out, the state is going to be rolled back. It simplifies the flow a bit. I can put it around an if(). 

If I'm parsing this correctly, you replied 'Done' wrt a few of Gabe's comments, but didn't update the diff... can you update the diff so I can see what the new diff looks like?

src/cpu/o3/iew_impl.hh (Diff revision 1)

This comment is not relevant to a review of this code at all, but I just have to point out that this is the first place I can recall that I've seen an explicit boolean comparison that makes sense... as Brad knows, explicit equality/inequality comparisons with true/false annoy me greatly as a huge redundancy, but given that "predicated false" is common terminology, this actually reads better than "if (!inst->readPredicate())".