> 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 ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1154/#review2569 ----------------------------------------------------------- On April 21, 2012, 1:11 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1154/ > ----------------------------------------------------------- > > (Updated April 21, 2012, 1:11 p.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 8959:c261b42c18bf > --------------------------- > ISA Parser: runtime read, write conditions for registers > This patch allows for operands to make run time decision, whether they > should be read/written at all. > > > Diffs > ----- > > src/arch/isa_parser.py 0bba1c59b4d1 > > Diff: http://reviews.gem5.org/r/1154/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
