Even if the named constant is in the same file just above where it is used, that is far better than using the same number in 50 different places. It is worth it because of exactly what's going on right now. You're submitting your 2nd 50 lines of diff to change one number in 50 places. Not cool.
On Tue, Nov 16, 2010 at 1:34 PM, Gabe Black <[email protected]> wrote: > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/312/ > > On November 15th, 2010, 9:46 p.m., *Nathan Binkert* wrote: > > > src/arch/arm/isa/operands.isa<http://reviews.m5sim.org/r/312/diff/1/?file=5179#file5179line89> > (Diff > revision 1) > > 89 > > 'FpDest': ('FloatReg', 'sf', '(dest + 0)', 'IsFloating', 3), > > 89 > > 'FpDest': ('FloatReg', 'sf', '(dest + 0)', 'IsFloating', 5), > > Ok, I thought I commented on this last time. This is nuts. You can't have > this random number here that needs to change whenever you do something. > > The number is for sorting the operands when they're written out/read in/etc. > It being a raw number is historical in two respects. First, that's how Alpha > did it, and that's how all the other ISAs inherited dong it. Second, the > operands construct used to be defined in an isolated scope, or in other words > the dict used as its global and/or local scope didn't have much in it, didn't > carry over from other parts of the description, etc. That made it essentially > impossible to use any variables defined elsewhere. That's no longer true, > though, since I opened it up to allow defining the shorthand functions in one > of the ISAs, although I forget which one. I think normally the numbers would > be sequential, but ARM has them set up to mostly not do anything. I half > remember that the only reason there isn't a single value all together is that > I needed to make sure the indexes were ordered in a particular way when > generating disassembly output, and then later to make sure interacting writes > happened in the right order. > > Anyway, I think using named constants would make sense here, although it goes > against the grain of the other ISAs and wouldn't necessarily fit with all of > them very well since they don't all reuse values like this. I'll make the > change before I check this in. > > > - Gabe > > On November 15th, 2010, 4:28 a.m., Gabe Black wrote: > Review request for Default. > By Gabe Black. > > *Updated 2010-11-15 04:28:11* > Description > > ARM: Take advantage of new PCState syntax. > > Diffs > > - src/arch/arm/isa/insts/branch.isa (f440cdaf1c2d) > - src/arch/arm/isa/insts/data.isa (f440cdaf1c2d) > - src/arch/arm/isa/insts/ldr.isa (f440cdaf1c2d) > - src/arch/arm/isa/insts/macromem.isa (f440cdaf1c2d) > - src/arch/arm/isa/insts/misc.isa (f440cdaf1c2d) > - src/arch/arm/isa/operands.isa (f440cdaf1c2d) > > View Diff <http://reviews.m5sim.org/r/312/diff/> >
_______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
