On 04/23/12 14:32, Nilay Vaish wrote: > >> On April 21, 2012, 8:08 p.m., Gabe Black wrote: >>> src/arch/isa_parser.py, line 525 >>> <http://reviews.gem5.org/r/1154/diff/3/?file=26101#file26101line525> >>> >>> 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. >> Nilay Vaish wrote: >> _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. >> >> Gabe Black wrote: >> 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. >> >> Nilay Vaish wrote: >> 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. >> >> Gabe Black wrote: >> 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. >> >> Gabe Black wrote: >> 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. >> >> Nilay Vaish wrote: >> I'll look into sub operand list, but it seems to me nothing has changed. > 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. > > > - Nilay >
We seem to whittled down my objections to a valid problem :-). One possibility which would change how the ISAs are implemented currently would be to have scalar members of the StaticInsts which implement the instructions which would hold the indexes to use for the various operands. These could be added programaticly like how the operands are declared in the various functions automatically. This has several benefits. First, it means the indexing problem is reduced because the indexes don't come out of the array any more. Second, it potentially makes it easier in, say, a disassembly function to determine which register indexes are which so that the right thing can be printed in the right position. This can be tricky when you can have things like condition codes, implicit registers, etc., getting mixed in with the more explicit sources and destinations. Third, it leads into another idea where these operand index members are actually objects which know how to read the right register bits for a read and write the right bits for a write without having to build those smarts into the parser. They could automatically populate the source and dest index arrays for the benefit of the CPU, but they would be what ultimately actually reads registers foo, bar, and baz and turns it into a register. That would be useful for the composite operands like a condition code register which is actually a bunch of smaller registers, or, say, a double precision floating point value which needs to read two singles to form a complete register. There are costs associated with doing things this way too, namely that a lot of code would have to be adjusted so it works this new way. It could be done in stages, though, at least during development, where each ISA could be converted individually. I think with the parser's help it could be somewhat mechanical as well. What do you think? Do you have another possible solution you'd like to propose? Gabe _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
