----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1166/#review2703 -----------------------------------------------------------
This seems to be basically going in the right direction, but there are a lot of rough edges to smooth out. src/arch/x86/isa/microops/debug.isa <http://reviews.gem5.org/r/1166/#comment3052> There should be spaces around |, here and below. src/arch/x86/isa/microops/regop.isa <http://reviews.gem5.org/r/1166/#comment3051> What are these constants? If this is splitting out the fields you mention in the commit message, that's already basically done. ccFlagBits means condition code flag bits. At the very least these constants should have names. Better yet, they should be derived from constants that already exist (ECFBit, EZFBit). src/arch/x86/isa/microops/regop.isa <http://reviews.gem5.org/r/1166/#comment3053> Don't put in a bunch of white space to make things line up. Tables are for data, not code. Unless you have a lot of data lining that up isn't all that useful either. src/arch/x86/isa/microops/regop.isa <http://reviews.gem5.org/r/1166/#comment3054> It looks like either you forgot to set the emulation flags here or you forgot to put this back on one line. Regardless, I think you should set EZF and ECF here to stay consistent with the old semantics. Things will need to be that way anyway once we have a bunch of registers. src/arch/x86/isa/microops/regop.isa <http://reviews.gem5.org/r/1166/#comment3055> Why the change in alignment? src/arch/x86/isa/operands.isa <http://reviews.gem5.org/r/1166/#comment3050> This name isn't great. At least leave Flag in there so I know what sort of bits they are (ucc by itself signifies almost nothing). - Gabe Black On May 16, 2012, 4:39 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1166/ > ----------------------------------------------------------- > > (Updated May 16, 2012, 4:39 p.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9008:ab37376c0044 > --------------------------- > X86: Split Condition Code register > This patch moves the ECF and EZF bits to a separate register. This is just > an example patch for discussion. This is how I am proposing we should split > the register. If this is acceptable, then ultimately we will have the > following registers -- [ZAPS], [OF], [CF], [ECF], [EZF] and [DF]. > > > Diffs > ----- > > src/arch/x86/isa/microops/debug.isa 7100059f7bfd > src/arch/x86/isa/microops/fpop.isa 7100059f7bfd > src/arch/x86/isa/microops/regop.isa 7100059f7bfd > src/arch/x86/isa/microops/seqop.isa 7100059f7bfd > src/arch/x86/isa/microops/specop.isa 7100059f7bfd > src/arch/x86/isa/operands.isa 7100059f7bfd > src/arch/x86/x86_traits.hh 7100059f7bfd > > Diff: http://reviews.gem5.org/r/1166/diff/ > > > Testing > ------- > > Boots Linux with atomic cpu. > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
