> 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.
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. - Gabe ----------------------------------------------------------- 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
