A meta-comment on the discussion (which also impacts the commit message): it would be nice if we could come up with a more precise term than "runtime" here. In a system where we run some code to generate other code that then gets compiled and run in order to decode binary instructions that are then themselves executed later, I can identify at least three points in the life of this code that could be called "run time".
We're really talking about allowing the number of registers for an operation represented by a particular C++ class to vary when that class is constructed. The number is still fixed for a given microop, or at least for a given instance of a microop. When I initially read this commit message it sounded like each time we execute a microop we dynamically figure out what set of registers it will read, which is a bit crazy. As Gabe mentioned previously, I think, it's arguably just a convenience that avoids having to define a ridiculous number of different microops for each possible set of flags that could be accessed. I'd say we're really letting the number of operands vary at decode time, as opposed to the previous situation, which is that the number never varied at all for a given C++ class. Steve On Sun, Apr 22, 2012 at 6:21 PM, Steve Reinhardt <[email protected]> wrote: > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1154/ > > Looks reasonable to me. There might be better names than "read_condition" > and "write_condition" though... particularly in the context of the current > discussion, it sounds like there's some tie-in with condition codes that > doesn't exist. What about "read_test" or "read_predicate"? > > > > src/arch/isa_parser.py<http://reviews.gem5.org/r/1154/diff/3/?file=26101#file26101line523> > (Diff > revision 3) > > def isReg(self): > > 523 > > if self.read_condition: > > Here and below, I'm fine with dropping the braces after the generated "if" > to avoid the second python "if" clause. I'm not so wedded to the style guide > that I think auto-generated code has to follow it even when it makes the > generator more complex. > > Alternatively, something like: > > c = '\n\t_srcRegIdx[] = ...;' > if self.read_condition: > c = 'if (%s) {\n %s\n}' % (self.reg_spec, c) > > would be IMO a cleaner way to put braces around the code. > > Or as a third alternative, make the generated "if" unconditional, but default > read_condition to "true" rather than None. > > Any of these would be preferable to me. The meta point is that simplicity & > readability of the code generator should trump simplicity & readability of > the generated code. > > > > src/arch/isa_parser.py<http://reviews.gem5.org/r/1154/diff/3/?file=26101#file26101line1805> > (Diff > revision 3) > > def buildOperandNameMap(self, user_dict, lineno): > > 1795 > > if len(val) > 6: > > 1805 > > if len(val) > 6: > > I'm sure there's a cleaner way to do this (not Nilay's fault, but the first > I've really seen this code, and Nilay is just making it worse). > > If None is the default value for all the optional attributes, you could do: > > val += [None]*4 # pad in case optional args are missing > base_cls_name, dflt_ext, reg_spec, flags, sort_pri, \ > read_code, write_code, read_condition, write_condition = val[:9] > > and get rid of all the "if"s. > > Or put parens around the LHS and you wouldn't need the backslash. > > > - Steve > > On April 21st, 2012, 1:11 p.m., Nilay Vaish wrote: > Review request for Default. > By Nilay Vaish. > > *Updated April 21, 2012, 1:11 p.m.* > 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) > > View Diff <http://reviews.gem5.org/r/1154/diff/> > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
