Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-06-13 Thread Gabe Black
On 06/12/11 16:29, Steve Reinhardt wrote:
 On Sun, Jun 12, 2011 at 1:05 AM, Gabe Black gbl...@eecs.umich.edu wrote:
 I was thinking about this today, and if we expand the read/write
 functions to handle signed types too, we're really just expanding the
 arbitrary set of types they can handle, not removing the limitation that
 you have to stay within those types which is what I think you don't like.
 I think originally we supported memory accesses for any operand type
 you could define, but that stopped being true once you made the
 definitions extensible.  My immediate concern is just to make sure
 that switching from the old explicit sign extensions to some implicit
 sign extensions that happened as a side effect of C type conversions
 is really doing the right thing, but having a cleaner way of doing
 memory accesses of arbitrary types is a good idea.

 Instead of extending what we already have as far as explicit
 instantiation, it would be nice to have a more automatic mechanism where
 we'd just feed a list of types and a template (you can pass templates as
 template arguments, sort of like function pointers but for templates)
 and have some widget that cranks out the actual instantiation without so
 much copy and paste coding.
 That sounds interesting, but seems like overkill... I just looked at
 the SimpleCPU code, and as far as I can tell, the memory access type
 (the arg type for read() and write()) is only used for two things: to
 determine the size of the access, and to control the data type in the
 InstRecord type for exec tracing (basically this is mostly setting the
 data_status enum, but also using the proper double vs int field in the
 data union).  The actual type clearly doesn't matter at all for the
 first, and only a subset of types are supported for the second.

There are actually three things, the third is to handle endianness. This
is important if we delegate endian swapping to the read and write
functions which we currently do, and helps when printing out what loads
and stores returned at the CPU level. You get the same information, it's
just easier to look at since it's a number and not a string of bytes.
Moving that into the ISA desc would be feasible, and you could make a
convincing argue that's where it should be in the first place. If ARM is
configured to do loads/stores in the other endianness (which we
support) then values could be swapped on the way in and then re-swapped
before being used. The idea of non-byte sized values in memory and the
endianness of them of is an ISA level concept. Other non-byte sized
values like PCI registers are another story, of course, but that's a
whole other issue.

 The original idea with the templates was that they might permit faster
 implementations for functional CPU models that communicated directly
 with memory.  However, if anything we've gone in the other direction
 by implementing these templates in terms of readBytes() and
 writeBytes().

 So my general feeling is that if we want to make significant changes
 to this interface, I'd be more inclined to streamline it and have the
 generated ISA code call readBytes() and writeBytes() directly with a
 size and some additional info to make exec tracing work (which should
 get rid of the templates entirely, I think) rather than expanding the
 template interface.  Then the burden of converting from an untyped
 sequence of bytes to whatever the ISA wants could be done entirely in
 the ISA definition, which seems like a good place for it.  Does that
 make sense?  Do you think it's feasible or worthwhile?

I think that makes sense, although the devil is in the details as
always. For memory in traces, it would be nice to have it endian
corrected, but it wouldn't necessarily have to be. We could just print
out each byte one at a time and have the understanding that it's a blob
of memory, low addresses on the left. Or at least I think we could do
that...

 Also, while looking for information about Boost (in progress right now)
 I found their page where they talk about their license (link below).
 Looking through it, there are some ideas there which seem relevant to
 gem5. Specifically, I like the idea of a single license for everything,
 perhaps involving assigning copyright to a neutral body like a gem5
 foundation or something, and then just referring to it in the actual
 source files. That would get rid of lots of redundant text, and they
 make a good point that all that text is the sort of thing lawyers might
 get their underwear in a bunch over. There may be (but isn't
 necessarily) subtle variation on a file by file basis, and it's probably
 a lot more work to go through if somebody ever needed to do that.
 We discussed this a long long time ago (when we first started
 distributing the code, IIRC), and while it does have the advantages
 you describe, the cost of further wrangling with lawyers is basically
 not worth it IMO.  Maybe if we started a new project from scratch we
 would consider it, 

Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-06-12 Thread Gabe Black
On 06/04/11 09:32, Steve Reinhardt wrote:
 On Sat, Jun 4, 2011 at 1:57 AM, Gabe Black gbl...@eecs.umich.edu wrote:
 To clarify, is this signed/unsigned issue something we need to deal with
 before this patch goes in, or can it be dealt with separately later?
 I'd like to see it handled before the patch is committed, mostly
 because I'm still not 100% convinced that these changes don't break
 something.  Also when something gets deferred like this there's always
 the uncertainty of when/if it's going to get taken care of... nothing
 personal, I'd feel the same way if it was my own patch.

 Steve
 ___
 gem5-dev mailing list
 gem5-dev@m5sim.org
 http://m5sim.org/mailman/listinfo/gem5-dev

I was thinking about this today, and if we expand the read/write
functions to handle signed types too, we're really just expanding the
arbitrary set of types they can handle, not removing the limitation that
you have to stay within those types which is what I think you don't like.

Instead of extending what we already have as far as explicit
instantiation, it would be nice to have a more automatic mechanism where
we'd just feed a list of types and a template (you can pass templates as
template arguments, sort of like function pointers but for templates)
and have some widget that cranks out the actual instantiation without so
much copy and paste coding. Some Googling indicates that Boost already
does this with some macros it has for working with sequences. I'm
looking into that more, and I'm thinking it would be best to figure out
how they did it and then make our own, rather than introduce a great big
new dependency.

Combining that with the first point, I think a nice solution would be to
have the ISA parser spit out a list of types that read/write need to
support in whatever form is necessary (preliminary reading suggests a
#define) and then have the CPU models all feed that into the
instantiator widget to get the versions they need. Then we'd get exactly
the functions we need and cut out a bunch of repetition in the source.
This would be at the reasonable cost (in my opinion) of some additional
complexity, although in some ways it would trade one type of complexity
for another.


Also, while looking for information about Boost (in progress right now)
I found their page where they talk about their license (link below).
Looking through it, there are some ideas there which seem relevant to
gem5. Specifically, I like the idea of a single license for everything,
perhaps involving assigning copyright to a neutral body like a gem5
foundation or something, and then just referring to it in the actual
source files. That would get rid of lots of redundant text, and they
make a good point that all that text is the sort of thing lawyers might
get their underwear in a bunch over. There may be (but isn't
necessarily) subtle variation on a file by file basis, and it's probably
a lot more work to go through if somebody ever needed to do that.

http://www.boost.org/users/license.html

Gabe
___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-06-12 Thread Steve Reinhardt
On Sun, Jun 12, 2011 at 1:05 AM, Gabe Black gbl...@eecs.umich.edu wrote:

 I was thinking about this today, and if we expand the read/write
 functions to handle signed types too, we're really just expanding the
 arbitrary set of types they can handle, not removing the limitation that
 you have to stay within those types which is what I think you don't like.

I think originally we supported memory accesses for any operand type
you could define, but that stopped being true once you made the
definitions extensible.  My immediate concern is just to make sure
that switching from the old explicit sign extensions to some implicit
sign extensions that happened as a side effect of C type conversions
is really doing the right thing, but having a cleaner way of doing
memory accesses of arbitrary types is a good idea.

 Instead of extending what we already have as far as explicit
 instantiation, it would be nice to have a more automatic mechanism where
 we'd just feed a list of types and a template (you can pass templates as
 template arguments, sort of like function pointers but for templates)
 and have some widget that cranks out the actual instantiation without so
 much copy and paste coding.

That sounds interesting, but seems like overkill... I just looked at
the SimpleCPU code, and as far as I can tell, the memory access type
(the arg type for read() and write()) is only used for two things: to
determine the size of the access, and to control the data type in the
InstRecord type for exec tracing (basically this is mostly setting the
data_status enum, but also using the proper double vs int field in the
data union).  The actual type clearly doesn't matter at all for the
first, and only a subset of types are supported for the second.

The original idea with the templates was that they might permit faster
implementations for functional CPU models that communicated directly
with memory.  However, if anything we've gone in the other direction
by implementing these templates in terms of readBytes() and
writeBytes().

So my general feeling is that if we want to make significant changes
to this interface, I'd be more inclined to streamline it and have the
generated ISA code call readBytes() and writeBytes() directly with a
size and some additional info to make exec tracing work (which should
get rid of the templates entirely, I think) rather than expanding the
template interface.  Then the burden of converting from an untyped
sequence of bytes to whatever the ISA wants could be done entirely in
the ISA definition, which seems like a good place for it.  Does that
make sense?  Do you think it's feasible or worthwhile?

 Also, while looking for information about Boost (in progress right now)
 I found their page where they talk about their license (link below).
 Looking through it, there are some ideas there which seem relevant to
 gem5. Specifically, I like the idea of a single license for everything,
 perhaps involving assigning copyright to a neutral body like a gem5
 foundation or something, and then just referring to it in the actual
 source files. That would get rid of lots of redundant text, and they
 make a good point that all that text is the sort of thing lawyers might
 get their underwear in a bunch over. There may be (but isn't
 necessarily) subtle variation on a file by file basis, and it's probably
 a lot more work to go through if somebody ever needed to do that.

We discussed this a long long time ago (when we first started
distributing the code, IIRC), and while it does have the advantages
you describe, the cost of further wrangling with lawyers is basically
not worth it IMO.  Maybe if we started a new project from scratch we
would consider it, but the horse has left the barn for gem5, I think.

Steve
___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-06-04 Thread Gabe Black
On 05/31/11 00:13, Gabe Black wrote:
 On 05/30/11 21:57, Steve Reinhardt wrote:
 On Mon, May 30, 2011 at 1:33 PM, Gabe Black gbl...@eecs.umich.edu wrote:
 On 05/30/11 09:47, Steve Reinhardt wrote:
 Anyway, it seems very odd to have to say (int8_t)Mem.ub when we already 
 have a .sb operand type defined as int8_t.  It seems like a weird 
 hidden restriction to say that there are operand types you can declare but 
 can't use on memory, and that you're pushing a somewhat arbitrary internal 
 implementation decision up to the level of language visibility, which is 
 going the wrong direction.  I'm not sure what the right solution is, but 
 even if it's the brute force one of creating a bunch more read/write 
 function templates in the CPU implementations, I think that's preferable.
 [...]

 The unsigned thing is sort of a weird gotcha. I'd like to avoid adding a
 lot of bulk to the CPU models since they're already fairly chunky and it
 makes them harder to reason about looking through the code, but it would
 be great to straighten this out. One possibility might be the technique
 I used with the endianness changing functions in src/sim/byteswap.hh.
 Things are built up in layers there:
 [...]
 I think some kind of additional set of template instantiations should
 do it.  I just noticed that we already have:

 template
 Fault
 AtomicSimpleCPU::read(Addr addr, int32_t data, unsigned flags)
 {
 return read(addr, (uint32_t)data, flags);
 }

 template
 Fault
 AtomicSimpleCPU::write(int32_t data, Addr addr, unsigned flags, uint64_t 
 *res)
 {
 return write((uint32_t)data, addr, flags, res);
 }

 .. and similar for TimingSimpleCPU; do we just need to extend these to
 int8_t and int16_t, and maybe add similar sets in
 base_dynamic_inst.hh?

 Steve
 ___
 gem5-dev mailing list
 gem5-dev@m5sim.org
 http://m5sim.org/mailman/listinfo/gem5-dev
 Oh yeah, I guess we're already doing something like that, and that glob
 of code was just instantiating different versions of the function. Could
 we reduce the amount of text by moving it into the header? There isn't a
 lot of actual information there in all that text, and we're talking
 about doubling it. I don't have a better idea, though.

 Gabe
 ___
 gem5-dev mailing list
 gem5-dev@m5sim.org
 http://m5sim.org/mailman/listinfo/gem5-dev

To clarify, is this signed/unsigned issue something we need to deal with
before this patch goes in, or can it be dealt with separately later?

Gabe
___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-06-04 Thread Steve Reinhardt
On Sat, Jun 4, 2011 at 1:57 AM, Gabe Black gbl...@eecs.umich.edu wrote:
 To clarify, is this signed/unsigned issue something we need to deal with
 before this patch goes in, or can it be dealt with separately later?

I'd like to see it handled before the patch is committed, mostly
because I'm still not 100% convinced that these changes don't break
something.  Also when something gets deferred like this there's always
the uncertainty of when/if it's going to get taken care of... nothing
personal, I'd feel the same way if it was my own patch.

Steve
___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-31 Thread Gabe Black
On 05/30/11 21:57, Steve Reinhardt wrote:
 On Mon, May 30, 2011 at 1:33 PM, Gabe Black gbl...@eecs.umich.edu wrote:
 On 05/30/11 09:47, Steve Reinhardt wrote:
 Anyway, it seems very odd to have to say (int8_t)Mem.ub when we already 
 have a .sb operand type defined as int8_t.  It seems like a weird 
 hidden restriction to say that there are operand types you can declare but 
 can't use on memory, and that you're pushing a somewhat arbitrary internal 
 implementation decision up to the level of language visibility, which is 
 going the wrong direction.  I'm not sure what the right solution is, but 
 even if it's the brute force one of creating a bunch more read/write 
 function templates in the CPU implementations, I think that's preferable.
 [...]

 The unsigned thing is sort of a weird gotcha. I'd like to avoid adding a
 lot of bulk to the CPU models since they're already fairly chunky and it
 makes them harder to reason about looking through the code, but it would
 be great to straighten this out. One possibility might be the technique
 I used with the endianness changing functions in src/sim/byteswap.hh.
 Things are built up in layers there:
 [...]
 I think some kind of additional set of template instantiations should
 do it.  I just noticed that we already have:

 template
 Fault
 AtomicSimpleCPU::read(Addr addr, int32_t data, unsigned flags)
 {
 return read(addr, (uint32_t)data, flags);
 }

 template
 Fault
 AtomicSimpleCPU::write(int32_t data, Addr addr, unsigned flags, uint64_t *res)
 {
 return write((uint32_t)data, addr, flags, res);
 }

 .. and similar for TimingSimpleCPU; do we just need to extend these to
 int8_t and int16_t, and maybe add similar sets in
 base_dynamic_inst.hh?

 Steve
 ___
 gem5-dev mailing list
 gem5-dev@m5sim.org
 http://m5sim.org/mailman/listinfo/gem5-dev

Oh yeah, I guess we're already doing something like that, and that glob
of code was just instantiating different versions of the function. Could
we reduce the amount of text by moving it into the header? There isn't a
lot of actual information there in all that text, and we're talking
about doubling it. I don't have a better idea, though.

Gabe
___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-30 Thread Gabe Black


 On 2011-05-28 21:58:27, Steve Reinhardt wrote:
  The description is a little general; can you be more specific about what 
  this change is doing?  I see that you're getting rid of the size from the 
  memory access operands and encoding that in the ctype instead, which seems 
  fine.  However it seems like you've gotten rid of a lot of the signed vs. 
  unsigned code, and made everything look unsigned, and I don't see how that 
  still works.

The reason I changed them to unsigned is that all the read/write functions have 
definitions generated for unsigned int operands but not signed ones. They were 
being forced to unsigned when the call was being made. All of the regressions 
passed with these changes and at the time I was convinced why this worked, but 
it's been about a month since then and I don't really remember the specifics. 
In SPARC and ARM, the Mem variables are being cast to an appropriate type in 
the assignment, and in Alpha there were apparently no signed Mem types. I'm 
guessing MIPS and Power don't have as extensive regressions so problems may 
have slipped through due to those instructions not being used or being used in 
a way that didn't expose a difference in behavior. The simple fix is to add 
casts to those too in the few places where it makes a difference, although 
there may be a reason that I'm not able to remember that it works as is.

Generally speaking, signed vs. unsigned, size, and float vs. int information 
only makes sense if you're building up a few preselected types as the code is 
currently doing. By deferring more to the C++ type system, you can use whatever 
type you want and it should just work.


- Gabe


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/655/#review1276
---


On 2011-04-25 03:03:53, Gabe Black wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/655/
 ---
 
 (Updated 2011-04-25 03:03:53)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 ISA parser: Simplify operand type handling.
 
 This change simplifies the code surrounding operand type handling and makes it
 depend only on the ctype that goes with each operand type. Future changes will
 allow defining operand types by their ctypes directly, convert the ISAs over
 to that style of definition, and then remove support for the old style. These
 changes are to make it easier to use non-builtin types like classes or
 structures as the type for operands.
 
 
 Diffs
 -
 
   src/arch/alpha/isa/mem.isa de679a068dd8 
   src/arch/arm/isa/insts/ldr.isa de679a068dd8 
   src/arch/arm/isa/insts/mem.isa de679a068dd8 
   src/arch/arm/isa/insts/str.isa de679a068dd8 
   src/arch/arm/isa/templates/mem.isa de679a068dd8 
   src/arch/isa_parser.py de679a068dd8 
   src/arch/mips/isa/decoder.isa de679a068dd8 
   src/arch/mips/isa/formats/mem.isa de679a068dd8 
   src/arch/power/isa/decoder.isa de679a068dd8 
   src/arch/power/isa/formats/mem.isa de679a068dd8 
   src/arch/sparc/isa/decoder.isa de679a068dd8 
   src/arch/sparc/isa/formats/mem/swap.isa de679a068dd8 
   src/arch/sparc/isa/formats/mem/util.isa de679a068dd8 
 
 Diff: http://reviews.m5sim.org/r/655/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Gabe
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-30 Thread Gabe Black

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/655/
---

(Updated 2011-05-30 00:18:31.344077)


Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan 
Binkert.


Summary
---

ISA parser: Simplify operand type handling.

This change simplifies the code surrounding operand type handling and makes it
depend only on the ctype that goes with each operand type. Future changes will
allow defining operand types by their ctypes directly, convert the ISAs over
to that style of definition, and then remove support for the old style. These
changes are to make it easier to use non-builtin types like classes or
structures as the type for operands.


Diffs (updated)
-

  src/arch/alpha/isa/mem.isa 03cfd2ecf6bb 
  src/arch/arm/isa/insts/ldr.isa 03cfd2ecf6bb 
  src/arch/arm/isa/insts/mem.isa 03cfd2ecf6bb 
  src/arch/arm/isa/insts/str.isa 03cfd2ecf6bb 
  src/arch/arm/isa/templates/mem.isa 03cfd2ecf6bb 
  src/arch/isa_parser.py 03cfd2ecf6bb 
  src/arch/mips/isa/decoder.isa 03cfd2ecf6bb 
  src/arch/mips/isa/formats/mem.isa 03cfd2ecf6bb 
  src/arch/power/isa/decoder.isa 03cfd2ecf6bb 
  src/arch/power/isa/formats/mem.isa 03cfd2ecf6bb 
  src/arch/sparc/isa/decoder.isa 03cfd2ecf6bb 
  src/arch/sparc/isa/formats/mem/swap.isa 03cfd2ecf6bb 
  src/arch/sparc/isa/formats/mem/util.isa 03cfd2ecf6bb 

Diff: http://reviews.m5sim.org/r/655/diff


Testing
---


Thanks,

Gabe

___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-30 Thread Steve Reinhardt


 On 2011-05-28 21:58:27, Steve Reinhardt wrote:
  The description is a little general; can you be more specific about what 
  this change is doing?  I see that you're getting rid of the size from the 
  memory access operands and encoding that in the ctype instead, which seems 
  fine.  However it seems like you've gotten rid of a lot of the signed vs. 
  unsigned code, and made everything look unsigned, and I don't see how that 
  still works.
 
 Gabe Black wrote:
 The reason I changed them to unsigned is that all the read/write 
 functions have definitions generated for unsigned int operands but not signed 
 ones. They were being forced to unsigned when the call was being made. All of 
 the regressions passed with these changes and at the time I was convinced why 
 this worked, but it's been about a month since then and I don't really 
 remember the specifics. In SPARC and ARM, the Mem variables are being cast to 
 an appropriate type in the assignment, and in Alpha there were apparently no 
 signed Mem types. I'm guessing MIPS and Power don't have as extensive 
 regressions so problems may have slipped through due to those instructions 
 not being used or being used in a way that didn't expose a difference in 
 behavior. The simple fix is to add casts to those too in the few places where 
 it makes a difference, although there may be a reason that I'm not able to 
 remember that it works as is.
 
 Generally speaking, signed vs. unsigned, size, and float vs. int 
 information only makes sense if you're building up a few preselected types as 
 the code is currently doing. By deferring more to the C++ type system, you 
 can use whatever type you want and it should just work.

Well, the fact that the old regressions didn't notice doesn't fill me with 
confidence, but at least it's more plausible that the updated version works.  I 
guess your old version may have worked in the MIPS case because the LHS is 
signed, e.g., lh is Rt.sw = Mem.uw while lhu is Rt.uw = Mem.uw, but the LHS 
is not signed in the Power case, and even in MIPS the size on the LHS for lb is 
wrong (it's also Rt.sw and not Rt.sb), which may or may not matter.

Anyway, it seems very odd to have to say (int8_t)Mem.ub when we already have 
a .sb operand type defined as int8_t.  It seems like a weird hidden 
restriction to say that there are operand types you can declare but can't use 
on memory, and that you're pushing a somewhat arbitrary internal implementation 
decision up to the level of language visibility, which is going the wrong 
direction.  I'm not sure what the right solution is, but even if it's the brute 
force one of creating a bunch more read/write function templates in the CPU 
implementations, I think that's preferable.

I must say that other than this one inconsistency, I like this change, and I'm 
left scratching my head about why I made the original version so complicated in 
the first place.


- Steve


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/655/#review1276
---


On 2011-05-30 00:18:31, Gabe Black wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/655/
 ---
 
 (Updated 2011-05-30 00:18:31)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 ISA parser: Simplify operand type handling.
 
 This change simplifies the code surrounding operand type handling and makes it
 depend only on the ctype that goes with each operand type. Future changes will
 allow defining operand types by their ctypes directly, convert the ISAs over
 to that style of definition, and then remove support for the old style. These
 changes are to make it easier to use non-builtin types like classes or
 structures as the type for operands.
 
 
 Diffs
 -
 
   src/arch/alpha/isa/mem.isa 03cfd2ecf6bb 
   src/arch/arm/isa/insts/ldr.isa 03cfd2ecf6bb 
   src/arch/arm/isa/insts/mem.isa 03cfd2ecf6bb 
   src/arch/arm/isa/insts/str.isa 03cfd2ecf6bb 
   src/arch/arm/isa/templates/mem.isa 03cfd2ecf6bb 
   src/arch/isa_parser.py 03cfd2ecf6bb 
   src/arch/mips/isa/decoder.isa 03cfd2ecf6bb 
   src/arch/mips/isa/formats/mem.isa 03cfd2ecf6bb 
   src/arch/power/isa/decoder.isa 03cfd2ecf6bb 
   src/arch/power/isa/formats/mem.isa 03cfd2ecf6bb 
   src/arch/sparc/isa/decoder.isa 03cfd2ecf6bb 
   src/arch/sparc/isa/formats/mem/swap.isa 03cfd2ecf6bb 
   src/arch/sparc/isa/formats/mem/util.isa 03cfd2ecf6bb 
 
 Diff: http://reviews.m5sim.org/r/655/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Gabe
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-30 Thread Gabe Black
On 05/30/11 09:47, Steve Reinhardt wrote:

 On 2011-05-28 21:58:27, Steve Reinhardt wrote:
 The description is a little general; can you be more specific about what 
 this change is doing?  I see that you're getting rid of the size from the 
 memory access operands and encoding that in the ctype instead, which seems 
 fine.  However it seems like you've gotten rid of a lot of the signed vs. 
 unsigned code, and made everything look unsigned, and I don't see how that 
 still works.
 Gabe Black wrote:
 The reason I changed them to unsigned is that all the read/write 
 functions have definitions generated for unsigned int operands but not 
 signed ones. They were being forced to unsigned when the call was being 
 made. All of the regressions passed with these changes and at the time I was 
 convinced why this worked, but it's been about a month since then and I 
 don't really remember the specifics. In SPARC and ARM, the Mem variables are 
 being cast to an appropriate type in the assignment, and in Alpha there were 
 apparently no signed Mem types. I'm guessing MIPS and Power don't have as 
 extensive regressions so problems may have slipped through due to those 
 instructions not being used or being used in a way that didn't expose a 
 difference in behavior. The simple fix is to add casts to those too in the 
 few places where it makes a difference, although there may be a reason that 
 I'm not able to remember that it works as is.
 
 Generally speaking, signed vs. unsigned, size, and float vs. int 
 information only makes sense if you're building up a few preselected types 
 as the code is currently doing. By deferring more to the C++ type system, 
 you can use whatever type you want and it should just work.
 Well, the fact that the old regressions didn't notice doesn't fill me with 
 confidence, but at least it's more plausible that the updated version works.  
 I guess your old version may have worked in the MIPS case because the LHS is 
 signed, e.g., lh is Rt.sw = Mem.uw while lhu is Rt.uw = Mem.uw, but the 
 LHS is not signed in the Power case, and even in MIPS the size on the LHS for 
 lb is wrong (it's also Rt.sw and not Rt.sb), which may or may not matter.

 Anyway, it seems very odd to have to say (int8_t)Mem.ub when we already 
 have a .sb operand type defined as int8_t.  It seems like a weird hidden 
 restriction to say that there are operand types you can declare but can't use 
 on memory, and that you're pushing a somewhat arbitrary internal 
 implementation decision up to the level of language visibility, which is 
 going the wrong direction.  I'm not sure what the right solution is, but even 
 if it's the brute force one of creating a bunch more read/write function 
 templates in the CPU implementations, I think that's preferable.

 I must say that other than this one inconsistency, I like this change, and 
 I'm left scratching my head about why I made the original version so 
 complicated in the first place.


 - Steve

I'm replying in email instead of on review board since this is more of a
general discussion than review feedback.

The unsigned thing is sort of a weird gotcha. I'd like to avoid adding a
lot of bulk to the CPU models since they're already fairly chunky and it
makes them harder to reason about looking through the code, but it would
be great to straighten this out. One possibility might be the technique
I used with the endianness changing functions in src/sim/byteswap.hh.
Things are built up in layers there:

The swapping functions that depend on the guest byte order are templated
on the type being swapped and are defined in terms of the functions that
use a fixed endianness on the guest end.

The functions that depend on the host and not the guest are templated on
the type as well, and use the preprocessor to pick versions that do
swapping or not based on the endianness of the host. They're defined
using the swap_byte function which is also templated on the type.

The functions that depend on neither the guest nor the host are defined
unconditionally, templated on the type, and use swap_byte for their
implementation.

The swap_byte function is templated on the type it's swapping. It's
implemented as an if that selects a swapping function based on the size
of the type involved. Because it's inlined and templated on a particular
type, the compiler should squash the if (and the function entirely) so
that the right thing is being called.

Then, finally, the swap_byteXX functions are called where XX is the
type's size in bits.

So with all these layers, the compiler can select the appropriate byte
swapping operation (or none at all) based on the starting and ending
orders, the host and guest orders, and the size of the type without
having to actually know about the type ahead of time, assuming it's 1,
2, 4, or 8 bytes in size.

We might be able to do something like this for the execution context
functions so that they can read anything as long as it's a recognized

Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-30 Thread Steve Reinhardt
On Mon, May 30, 2011 at 1:33 PM, Gabe Black gbl...@eecs.umich.edu wrote:
 On 05/30/11 09:47, Steve Reinhardt wrote:

 Anyway, it seems very odd to have to say (int8_t)Mem.ub when we already 
 have a .sb operand type defined as int8_t.  It seems like a weird hidden 
 restriction to say that there are operand types you can declare but can't 
 use on memory, and that you're pushing a somewhat arbitrary internal 
 implementation decision up to the level of language visibility, which is 
 going the wrong direction.  I'm not sure what the right solution is, but 
 even if it's the brute force one of creating a bunch more read/write 
 function templates in the CPU implementations, I think that's preferable.
 [...]

 The unsigned thing is sort of a weird gotcha. I'd like to avoid adding a
 lot of bulk to the CPU models since they're already fairly chunky and it
 makes them harder to reason about looking through the code, but it would
 be great to straighten this out. One possibility might be the technique
 I used with the endianness changing functions in src/sim/byteswap.hh.
 Things are built up in layers there:
 [...]

I think some kind of additional set of template instantiations should
do it.  I just noticed that we already have:

template
Fault
AtomicSimpleCPU::read(Addr addr, int32_t data, unsigned flags)
{
return read(addr, (uint32_t)data, flags);
}

template
Fault
AtomicSimpleCPU::write(int32_t data, Addr addr, unsigned flags, uint64_t *res)
{
return write((uint32_t)data, addr, flags, res);
}

.. and similar for TimingSimpleCPU; do we just need to extend these to
int8_t and int16_t, and maybe add similar sets in
base_dynamic_inst.hh?

Steve
___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.

2011-05-28 Thread Steve Reinhardt

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/655/#review1276
---


The description is a little general; can you be more specific about what this 
change is doing?  I see that you're getting rid of the size from the memory 
access operands and encoding that in the ctype instead, which seems fine.  
However it seems like you've gotten rid of a lot of the signed vs. unsigned 
code, and made everything look unsigned, and I don't see how that still works.


src/arch/mips/isa/decoder.isa
http://reviews.m5sim.org/r/655/#comment1753

This confuses me... how is it that lb  lbu (and lh  lhu) have identical 
definitions now?  What is it that makes lb and lh signed?


- Steve


On 2011-04-25 03:03:53, Gabe Black wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/655/
 ---
 
 (Updated 2011-04-25 03:03:53)
 
 
 Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
 Nathan Binkert.
 
 
 Summary
 ---
 
 ISA parser: Simplify operand type handling.
 
 This change simplifies the code surrounding operand type handling and makes it
 depend only on the ctype that goes with each operand type. Future changes will
 allow defining operand types by their ctypes directly, convert the ISAs over
 to that style of definition, and then remove support for the old style. These
 changes are to make it easier to use non-builtin types like classes or
 structures as the type for operands.
 
 
 Diffs
 -
 
   src/arch/alpha/isa/mem.isa de679a068dd8 
   src/arch/arm/isa/insts/ldr.isa de679a068dd8 
   src/arch/arm/isa/insts/mem.isa de679a068dd8 
   src/arch/arm/isa/insts/str.isa de679a068dd8 
   src/arch/arm/isa/templates/mem.isa de679a068dd8 
   src/arch/isa_parser.py de679a068dd8 
   src/arch/mips/isa/decoder.isa de679a068dd8 
   src/arch/mips/isa/formats/mem.isa de679a068dd8 
   src/arch/power/isa/decoder.isa de679a068dd8 
   src/arch/power/isa/formats/mem.isa de679a068dd8 
   src/arch/sparc/isa/decoder.isa de679a068dd8 
   src/arch/sparc/isa/formats/mem/swap.isa de679a068dd8 
   src/arch/sparc/isa/formats/mem/util.isa de679a068dd8 
 
 Diff: http://reviews.m5sim.org/r/655/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Gabe
 


___
gem5-dev mailing list
gem5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/gem5-dev