> On Aug. 26, 2012, 4:06 p.m., Steve Reinhardt wrote: > > Sorry for the delay on this one, Nilay... it looks good. > > > > I'm still not thrilled with the amount of duplication in the makeRead() and > > makeWrite() methods though. I'm fine with having all the instructions use > > _numSrcRegs++ / _numDestRegs++ as the indices, similar to the way This > > would allow you to get rid of the predRead and predWrite flags entirely in > > addition to simplifying the code in makeRead() and makeWrite(). There > > could be a small performance penalty in debug mode (then again, there might > > not be or it might be negligible), but I'd be very disappointed if the > > compiler didn't optimize the difference away entirely for opt or fast. To > > me that's a reasonable tradeoff. > > > > If you really don't want to go there, then one thing you could do that > > would clean up the code a bit would be to set src_reg_idx to > > '_sourceIndex++' and dst_reg_idx to '_destIndex++' for operands that are in > > predicated instructions. Then in makeRead() and makeWrite() you could > > unconditionally do things like this: > > > > int_reg_val = 'xc->readIntRegOperand(this, %s)' % self.src_reg_idx > > > > Note that in python '%s' and '%d' do the same thing if the corresponding > > variable is an integer. > > > > This would at least get rid of a lot of the redundancy in the generated > > code strings, which is especially bad in makeWrite(). It comes at the cost > > of complicating OperandList.__init__() a bit, since you'd have to make an > > initial pass to see if predRead or predWrite would be set before being able > > to set src_reg_idx/dst_reg_idx properly (or you could use a second pass to > > override src_reg_idx/dst_reg_idx if predRead/predWrite are set). > > > > Does that make sense? I'd prefer the former idea (just making everything > > uniform), but I threw the second one out there as a compromise to be able > > to generate the same output you're generating now only (IMO) just a little > > more cleanly.
Steve, are you statisfied with patch now? - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1153/#review3341 ----------------------------------------------------------- On Aug. 27, 2012, 8:12 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1153/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2012, 8:12 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9173:ae0ade8173ef > --------------------------- > ISA Parser: Allow predication of source and destination registers > This patch is meant for allowing predicated reads and writes. Note that this > predication is different from the ISA provided predication. They way we > currently provide the ISA description for X86, we read/write registers that > do not need to be actually read/written. This is likely to be true for other > ISAs as well. This patch allows for read and write predicates to be associated > with operands. It allows for the register indices for source and destination > registers to be decided at the time when the microop is constructed. The > run time indicies come in to play only when the at least one of the > predicates has been provided. This patch will not affect any of the ISAs that > do not provide these predicates. Also the patch assumes that the order in > which operands appear in any function of the microop is same across all the > functions of the microops. A subsequent patch will enable predication for the > x86 ISA. > > > Diffs > ----- > > src/arch/isa_parser.py 2f2e4582014f > > Diff: http://reviews.gem5.org/r/1153/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
