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