arch, cpu: Architectural Register structural indexing

Information
Submitter:	Andreas Sandberg
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11464:d0b3c18e9e75
---------------------------
arch, cpu: Architectural Register structural indexing

Replace the unified register mapping by a structure associating a
class and an index. It is now much easier to know which class of
register the index is refering to.  Also, when adding a new class
there is no need to modify existing ones.

Change-Id: I55b3ac80763702aa2cd3ed2cbff0a75ef7620373
Reviewed-by: Andreas Sandberg <andreas.sandberg@arm.com>

Testing Done

Issue Summary

Description	From	Last Updated	Status
would be more compact (and more consistent with other functions, not to mention potentially more efficient) to just define a ...	Steve Reinhardt	May 24, 2016, 2:51 p.m.	Resolved
why the extra parens?	Steve Reinhardt	May 24, 2016, 2:51 p.m.	Resolved
Since these strings get used over and over, I'd say we should create some module-level string variables, e.g.: src_reg_constructor = ...	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved
would be nice to get rid of this eventually... please add a TODO comment explaining why it's too daunting to ...	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved
could use the struct directly, I think, e.g.: for (RegId reg_id(IntRegClass, 0); reg_id.regIdx < TheISA::NumIntRegs; reg_id.regIdx++) { [...] } not ...	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved
use initializer list : regClass(reg_class), regIdx(reg_idx)	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved
blank lines between methods (here & below)	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved
I find return !(*this==that); more intuitive (subjective though)	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved
I would prefer return (regIdx == TheISA::ZeroReg && (regClass == IntRegClass \|\| (THE_ISA == ALPHA_ISA && regClass == FloatRegClass)));	Steve Reinhardt	May 24, 2016, 2:52 p.m.	Resolved

Nice! It's great to see someone unafraid to take on big cleanup tasks like this. Several little things below that could be tweaked, but on the whole it's a very positive change! Thanks!

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)

Thanks for your review!
I've fixed most of the things you pointed out. See the comments for the details.

src/arch/arm/insts/branch64.cc (Diff revision 1)

would be more compact (and more consistent with other functions, not to mention potentially more efficient) to just define a printIntReg() function rather than having to explicitly add IntRegClass here, then test the value later

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)

printIntReg() / printFloatReg() / printMiscReg() / printCCReg() functions have been added.

The code has been modified to use them.

src/arch/arm/insts/static_inst.cc (Diff revision 1)

this is the switch you could get rid of if you switched to separate printIntReg() / printFloatReg() / printMiscReg() / printCCReg() functions

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Done.
```

src/arch/isa_parser.py (Diff revision 1)

why the extra parens?

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Fixed.
```

src/arch/isa_parser.py (Diff revision 1)

Since these strings get used over and over, I'd say we should create some module-level string variables, e.g.:
src_reg_constructor = '\n\t_srcRegIdx[_numSrcRegs++] = RegId(%s, %s);'
In fact, if we create a couple of class-level attributes for reg_class and reg_class_counter (e.g., '_numIntDestRegs'), then we can probably define a single common makeConstructor() method for the Operand class

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)

src_reg_constructor and dst_reg_constructor have been added and are used where it makes sense.
reg_class is now a class-level attribute.
Complete refactoring of the makeConstructor() is more difficult because not all operands do exactly the same things for it. So it will to be addressed in an other patch.

src/arch/mips/isa/decoder.isa (Diff revision 1)

might be worth defining a macro or helper function to clean this up... then again maybe not.

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)

I'm not really sure on how to define macro in .isa file.
And it would be a bit outside of the scope of this change.

src/arch/x86/insts/static_inst.hh (Diff revision 1)

would be nice to get rid of this eventually... please add a TODO comment explaining why it's too daunting to do here :)

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)

TODO added.

To be fair, I have tried to not define this function in the first place. But my understand of the ISA definition in X86 is too limited for that.

src/cpu/o3/cpu.cc (Diff revision 1)

could use the struct directly, I think, e.g.:
for (RegId reg_id(IntRegClass, 0); reg_id.regIdx < TheISA::NumIntRegs;
     reg_id.regIdx++) {
    [...]
}

not a huge deal here, but should avoid breaking the assignment across multiple lines in removeThread() below

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Done.
```

src/cpu/o3/cpu.cc (Diff revision 1)

if for some reason you do end up needing to break an assignment like this across two lines, I think it's cleaner to break it at the '='

src/cpu/reg_class.hh (Diff revision 1)

use initializer list
: regClass(reg_class), regIdx(reg_idx)

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Done.
```

src/cpu/reg_class.hh (Diff revision 1)

blank lines between methods (here & below)

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Done.
```

src/cpu/reg_class.hh (Diff revision 1)

I find

     return !(*this==that);

more intuitive (subjective though)

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Both are ok for me so I've picked yours.
```

src/cpu/reg_class.hh (Diff revision 1)

I would prefer
    return (regIdx == TheISA::ZeroReg &&
            (regClass == IntRegClass ||
             (THE_ISA == ALPHA_ISA && regClass == FloatRegClass)));

Show all issues

Nathanael Premillieu May 23, 2016, 9:48 a.m. (May 23, 2016, 9:48 a.m.)
```
Fixed.
```

Description:

~		Changeset 11464:810b3115a5d6
	~	Changeset 11464:d0b3c18e9e75

		arch, cpu: Architectural Register structural indexing

~		Replace the unified register mapping by a structure
~		associating a class and an index. It is now much
~		easier to know which class of register the index is refering to.
~		Also, when adding a new class there is no need to modify
	~	Replace the unified register mapping by a structure associating a
	~	class and an index. It is now much easier to know which class of
	~	register the index is refering to. Also, when adding a new class
	~	there is no need to modify existing ones.
-		existing ones.

		Change-Id: I55b3ac80763702aa2cd3ed2cbff0a75ef7620373
~		Signed-off-by: Andreas Sandberg andreas.sandberg@arm.com
	~	Reviewed-by: Andreas Sandberg andreas.sandberg@arm.com

Diff:

Revision 2 (+840 -773)

Show changes

	src/arch/isa_parser.py
	src/arch/alpha/registers.hh
	src/arch/alpha/isa/branch.isa
	src/arch/alpha/isa/fp.isa
	src/arch/alpha/isa/main.isa
	src/arch/arm/registers.hh
	src/arch/arm/insts/branch64.cc
	src/arch/arm/insts/data64.cc
	59 more

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Issue Summary

Description:

Changeset 11464:810b3115a5d6

Changeset 11464:d0b3c18e9e75

Diff: