----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1154/#review2591 -----------------------------------------------------------
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/#comment2985> 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/#comment2986> 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 Reinhardt 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
