In concept this is ok, but I don't think it will work for reasons I've mentioned inline. Even if it's a really artificial test, you should be sure to try out these sorts of things and make sure they do what you expect. There are lots of intricacies in the parser that are easy to miss.

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

What's preventing _numSrcRegs from already including this register? Also, there's a mapping between the operand number and this array. If the numbering is off because something got skipped, then you'll read the wrong registers.

Show all issues

1. Nilay Vaish April 21, 2012, 2:25 p.m. (April 21, 2012, 2:25 p.m.)

   _numSrcRegs will be set to 0 to begin with. With in the constructor
   of the microop, its correct value will be arrived depending on how
   many registers are required to be read. The array itself is being
   built in the constructor, so I do not expect any register to be
   missed.

2. Gabe Black April 21, 2012, 9:13 p.m. (April 21, 2012, 9:13 p.m.)

       def makeRead(self):
           if (self.ctype == 'float' or self.ctype == 'double'):
               error('Attempt to read integer register as FP')
           if self.read_code != None:
               return self.buildReadCode('readIntRegOperand')
           int_reg_val = 'xc->readIntRegOperand(this, %d)' % self.src_reg_idx
           return '%s = %s;\n' % (self.base_name, int_reg_val)
   
   In that code, the instruction will call readIntRegOperand to get the value to use for a particular register. src_reg_idx is determined before compile time, and readIntRegOperand uses the register indexes in that array, indexed by src_reg_idx, to actually do the read.

3. Nilay Vaish April 21, 2012, 11:52 p.m. (April 21, 2012, 11:52 p.m.)

   I know this. You may note that makeRead() has been changed in a similar fashion 
   so that indices are correctly computed while reading the registers.

4. Gabe Black April 22, 2012, 6:24 a.m. (April 22, 2012, 6:24 a.m.)

   I don't think that actually solves the problem for several reasons. First, it assumes that the registers will always be read in the same order they're processed in the constructor. That's true now, but it makes the code more fragile. Second, if the read or write code is overwritten, it's possible the new version would use the source index srcRegIndex++ more than once and get two different answers. You haven't actually updated that mechanism as far as I see, so it would be actually be using the old system which would be even more off. Third, all the registers are not always used in a given piece of code. There are cases where an instruction has multiple functions associated with it. The registers in the constructor are the union of the sets of registers used in each function. I can't point to a specific example at the moment, but that's why all that SubOperandList stuff was added to the parser.

5. Gabe Black April 22, 2012, 6:50 a.m. (April 22, 2012, 6:50 a.m.)

   Ok, I see why the srcRegIndex++ thing itself isn't a problem. I don't know why I didn't see that before. In any case, the part about not all registers being read every time is still true though.

6. Nilay Vaish April 22, 2012, 12:52 p.m. (April 22, 2012, 12:52 p.m.)

   I'll look into sub operand list, but it seems to me nothing has changed.

7. Nilay Vaish April 23, 2012, 7:32 a.m. (April 23, 2012, 7:32 a.m.)

   In the sparc isa, there are some instructions that will have problem
   with proposed changes. To over come the problem, we can introduce a mapping from
   the original order of the operands (as discovered by the isa parser) to the run time
   order of the operands. This mapping is created with in the constructor of the microop.
   When executing the microop, the mapping will be used for reading the operand.
   
   It seems that this would take care of the issue, but changes would have to be made to
   the arm isa. In the isa files for arm, at times source and destination register indices
   are built with in the isa file, instead of letting the isa parser generate that code.
   So this mapping would also have to be done in the isa files.

Looks reasonable to me.  There might be better names than "read_condition" and "write_condition" though... particularly in the context of the current discussion, it sounds like there's some tie-in with condition codes that doesn't exist.  What about "read_test" or "read_predicate"?

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

Here and below, I'm fine with dropping the braces after the generated "if" to avoid the second python "if" clause.  I'm not so wedded to the style guide that I think auto-generated code has to follow it even when it makes the generator more complex.

Alternatively, something like:

c = '\n\t_srcRegIdx[] = ...;'
if self.read_condition:
    c = 'if (%s) {\n    %s\n}' % (self.reg_spec, c)

would be IMO a cleaner way to put braces around the code.

Or as a third alternative, make the generated "if" unconditional, but default read_condition to "true" rather than None.

Any of these would be preferable to me.  The meta point is that simplicity & readability of the code generator should trump simplicity & readability of the generated code.

Show all issues

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

I'm sure there's a cleaner way to do this (not Nilay's fault, but the first I've really seen this code, and Nilay is just making it worse).

If None is the default value for all the optional attributes, you could do:

val += [None]*4  # pad in case optional args are missing
base_cls_name, dflt_ext, reg_spec, flags, sort_pri, \
read_code, write_code, read_condition, write_condition = val[:9]

and get rid of all the "if"s.

Or put parens around the LHS and you wouldn't need the backslash.

Show all issues