arch: Allow named constants as decode case values.

Information
Submitter:	Gabe Black
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10590:1c185336bb80
---------------------------
arch: Allow named constants as decode case values.

The values in a "bitfield" or in an ExtMachInst structure member may not be a
literal value, it might select from an arbitrary collection of options. Instead
of using the raw value of those constants in the decoder, it's easier to tell
what's going on if they can be referred to as a symbolic constant/enum.

To support that, the ISA description language is extended slightly so that in
addition to integer literals, the case value for decode blobs can also be a
string literal. It's up to the ISA author to ensure that the string evaluates
to a legal constant value when interpretted as C++.

Testing Done

Per the review process documentation on the wiki, if nobody comments on this or asks me to wait after 10 days, I'm going to commit this as is.

ISA is not a keyword at the moment (perhaps it should be), as per http://www.gem5.org/Commit_Access

For the rest it looks good from my side.

Gabe Black Nov. 23, 2014, 6:50 a.m. (Nov. 23, 2014, 6:50 a.m.)

In my defense there didn't use to be a list. I'll use arch I guess.

Summary:

-	ISA: Allow named constants as decode case values.
+	arch: Allow named constants as decode case values.

Description:

~		Changeset 10539:db6cb6e4303b
	~	Changeset 10539:6785122e7e3b

~		ISA: Allow named constants as decode case values.
	~	arch: Allow named constants as decode case values.

		The values in a "bitfield" or in an ExtMachInst structure member may not be a
		literal value, it might select from an arbitrary collection of options. Instead
		of using the raw value of those constants in the decoder, it's easier to tell
		what's going on if they can be referred to as a symbolic constant/enum.

		To support that, the ISA description language is extended slightly so that in
		addition to integer literals, the case value for decode blobs can also be a
		string literal. It's up to the ISA author to ensure that the string evaluates
		to a legal constant value when interpretted as C++.

Diff:

Revision 2 (+35 -29)

Show changes

src/arch/isa_parser.py

Please take a look at 2506 as well.

I'd suggest to wait and see what the others have to say as well.

Looks good, just some minor aesthetics

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

update comment

Gabe Black Dec. 3, 2014, 3:56 a.m. (Dec. 3, 2014, 3:56 a.m.)

I assume you mean just making it plural? The values do still have to be constant or C++ will get angry.

Steve Reinhardt Dec. 4, 2014, 7:38 a.m. (Dec. 4, 2014, 7:38 a.m.)

yea, something like "matches any of the specified constant(s)" or "matches a member of the constant list"

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

it would be nice to update this variable name too ('case_list'?)

Gabe Black Dec. 3, 2014, 3:56 a.m. (Dec. 3, 2014, 3:56 a.m.)
```
How about decode_vals? I'm open to suggestions.
```

Steve Reinhardt Dec. 4, 2014, 7:36 a.m. (Dec. 4, 2014, 7:36 a.m.)

sorry for the misunderstanding... the 'case_list' in the grammar expression is just fine, I was suggesting that the 'label' variable in the code block also be renamed to 'case_list' to match the grammar expression

Gabe Black Dec. 4, 2014, 3:43 p.m. (Dec. 4, 2014, 3:43 p.m.)

Ok, I changed decode_vals back to case_list, changed the variable name, and I'll push it like that.

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

same comment re: variable name

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

seems like we could still use a comment about 'default', or maybe just take the comment above p_case_list_1 and move it up here and add a mention of 'default'

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

do you really mean "C++ symbols" not "C++ strings"?

Steve Reinhardt Dec. 1, 2014, 10:20 p.m. (Dec. 1, 2014, 10:20 p.m.)

OK, I see from the other patch that it really is strings.  If you put in a string that doesn't evaluate to a constant, is that caught by the C++ compiler?  If so, it might be more accurate to say "strings (which must evaluate to constants to avoid a compilation error)" rather than just "strings which evaluate to constants"

Gabe Black Dec. 3, 2014, 3:56 a.m. (Dec. 3, 2014, 3:56 a.m.)

I changed it to "strings which evaluate to constants when compiled as C++"

Steve Reinhardt Dec. 4, 2014, 7:36 a.m. (Dec. 4, 2014, 7:36 a.m.)
```
sounds good, thanks
```

Description:

~		Changeset 10539:6785122e7e3b
	~	Changeset 10590:1c185336bb80

		arch: Allow named constants as decode case values.

		The values in a "bitfield" or in an ExtMachInst structure member may not be a
		literal value, it might select from an arbitrary collection of options. Instead
		of using the raw value of those constants in the decoder, it's easier to tell
		what's going on if they can be referred to as a symbolic constant/enum.

		To support that, the ISA description language is extended slightly so that in
		addition to integer literals, the case value for decode blobs can also be a
		string literal. It's up to the ISA author to ensure that the string evaluates
		to a legal constant value when interpretted as C++.

Diff:

Revision 3 (+37 -30)

Show changes

src/arch/isa_parser.py

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Summary:

Description:

Changeset 10539:db6cb6e4303b

Changeset 10539:6785122e7e3b

Diff:

Description:

Changeset 10539:6785122e7e3b

Changeset 10590:1c185336bb80

Diff:

Status: Closed (submitted)