Re: [gem5-dev] Review Request: ISA parser: Simplify operand type handling.
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: X86: Convert operand types to the new style.
On 2011-05-28 22:04:57, Steve Reinhardt wrote: Looks fine, but shouldn't you be doing all the ISAs if you're going to get rid of the old style? I have, but I didn't post all the patches since they're pretty similar and I didn't want to clutter up review board. I mentioned that when I first posted these, but that was a while ago. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/657/#review1278 --- On 2011-04-25 03:04:20, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/657/ --- (Updated 2011-04-25 03:04:20) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86: Convert operand types to the new style. Diffs - src/arch/x86/isa/operands.isa de679a068dd8 Diff: http://reviews.m5sim.org/r/657/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: Stop supporting the old style operand types.
On 2011-05-28 22:09:01, Steve Reinhardt wrote: See previous reviews... this looks fine if all the ISAs are converted to use it. Once it's committed, the relevant documentation should also be updated (e.g., http://gem5.org/Code_parsing#Operand_type_qualifiers). Will do. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/658/#review1279 --- On 2011-04-25 03:04:33, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/658/ --- (Updated 2011-04-25 03:04:33) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ISA parser: Stop supporting the old style operand types. Diffs - src/arch/isa_parser.py de679a068dd8 Diff: http://reviews.m5sim.org/r/658/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.
--- 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
[gem5-dev] Cron m5test@zizzer /z/m5/regression/do-regression quick
* build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/simple-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/simple-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/o3-timing passed. * build/ALPHA_SE/tests/opt/quick/20.eio-short/alpha/eio/simple-timing passed. * build/ALPHA_SE_MESI_CMP_directory/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MESI_CMP_directory passed. * build/ALPHA_SE_MOESI_CMP_directory/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_directory passed. * build/ALPHA_SE_MOESI_CMP_directory/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_directory passed. * build/ALPHA_SE_MOESI_CMP_directory/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_directory passed. * build/ALPHA_SE_MESI_CMP_directory/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MESI_CMP_directory passed. * build/ALPHA_SE_MESI_CMP_directory/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MESI_CMP_directory passed. * build/ALPHA_SE/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby passed. * build/ALPHA_SE/tests/opt/quick/01.hello-2T-smt/alpha/linux/o3-timing passed. * build/ALPHA_SE/tests/opt/quick/20.eio-short/alpha/eio/simple-atomic passed. * build/ALPHA_SE/tests/opt/quick/30.eio-mp/alpha/eio/simple-timing-mp passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/simple-atomic passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/inorder-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/simple-atomic passed. * build/ALPHA_SE/tests/opt/quick/30.eio-mp/alpha/eio/simple-atomic-mp passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/o3-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MOESI_hammer passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MOESI_hammer passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MOESI_hammer passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_token passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_token passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_token passed. * build/ALPHA_SE/tests/opt/quick/50.memtest/alpha/linux/memtest-ruby passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-timing passed. * build/ALPHA_FS/tests/opt/quick/80.netperf-stream/alpha/linux/twosys-tsunami-simple-atomic passed. * build/ALPHA_SE/tests/opt/quick/50.memtest/alpha/linux/memtest passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-timing-dual passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/50.memtest/alpha/linux/memtest-ruby-MOESI_hammer passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-atomic-dual passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-atomic passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/inorder-timing passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/o3-timing passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/simple-timing-ruby passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/simple-atomic passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/simple-timing passed. * build/POWER_SE/tests/opt/quick/00.hello/power/linux/simple-atomic passed. * build/POWER_SE/tests/opt/quick/00.hello/power/linux/o3-timing passed. * build/SPARC_SE/tests/opt/quick/02.insttest/sparc/linux/simple-atomic passed. * build/SPARC_SE/tests/opt/quick/40.m5threads-test-atomic/sparc/linux/simple-timing-mp passed. * build/SPARC_SE/tests/opt/quick/40.m5threads-test-atomic/sparc/linux/simple-atomic-mp passed. * build/X86_SE/tests/opt/quick/00.hello/x86/linux/simple-timing passed. * build/X86_SE/tests/opt/quick/00.hello/x86/linux/simple-timing-ruby passed. * build/X86_SE/tests/opt/quick/00.hello/x86/linux/simple-atomic passed. * build/X86_SE/tests/opt/quick/00.hello/x86/linux/o3-timing passed. * build/SPARC_SE/tests/opt/quick/00.hello/sparc/linux/simple-timing-ruby passed. * build/SPARC_SE/tests/opt/quick/02.insttest/sparc/linux/o3-timing passed. * build/SPARC_SE/tests/opt/quick/02.insttest/sparc/linux/simple-timing passed. * build/SPARC_SE/tests/opt/quick/00.hello/sparc/linux/simple-atomic passed. * build/SPARC_SE/tests/opt/quick/40.m5threads-test-atomic/sparc/linux/o3-timing-mp passed.
Re: [gem5-dev] Review Request: ISA parser: Allow defining operand types with a ctype directly.
On 2011-05-28 22:04:00, Steve Reinhardt wrote: This looks fine to me, but I'm confused... don't you delete this code completely two patches from now? Why bother changing it if you're going to get rid of it? Gabe Black wrote: Because this way both are available, and the ISAs can be converted one at a time and everything will work between every patch. Otherwise I'd have to break everything that wasn't (or was) updated, or do everything in one big change that does too much at once. Once all the ISAs are consistently on the new system, the old system isn't needed anymore and is deleted in that later patch. OK, I see. Overall this seems like overkill; I can see how this code was useful while you were developing, but for committing, one complete patch that gets rid of the old way of doing things and fixes all the ISAs to use the new way would be preferable to me. I think it would be less confusing because all the related changes would be right there in one place. I don't have a problem with large changesets (in terms of lines of code or files touched), just ones that are not conceptually unified, and spreading this change across multiple csets goes against conceptually unified in the opposite direction, IMO. - Steve --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/#review1277 --- On 2011-04-25 03:04:12, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/ --- (Updated 2011-04-25 03:04:12) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ISA parser: Allow defining operand types with a ctype directly. Diffs - src/arch/isa_parser.py de679a068dd8 Diff: http://reviews.m5sim.org/r/656/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.
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: Allow defining operand types with a ctype directly.
On 2011-05-28 22:04:00, Steve Reinhardt wrote: This looks fine to me, but I'm confused... don't you delete this code completely two patches from now? Why bother changing it if you're going to get rid of it? Gabe Black wrote: Because this way both are available, and the ISAs can be converted one at a time and everything will work between every patch. Otherwise I'd have to break everything that wasn't (or was) updated, or do everything in one big change that does too much at once. Once all the ISAs are consistently on the new system, the old system isn't needed anymore and is deleted in that later patch. Steve Reinhardt wrote: OK, I see. Overall this seems like overkill; I can see how this code was useful while you were developing, but for committing, one complete patch that gets rid of the old way of doing things and fixes all the ISAs to use the new way would be preferable to me. I think it would be less confusing because all the related changes would be right there in one place. I don't have a problem with large changesets (in terms of lines of code or files touched), just ones that are not conceptually unified, and spreading this change across multiple csets goes against conceptually unified in the opposite direction, IMO. That makes sense, but then I think when changes get to be too big, they get to be too hard to understand all at once. By keeping each part relatively small it makes things easier to digest later. My enormous PC state change is an example of the opposite extreme, and it was probably a lot of work to get through and review and overwhelming to look at later in the history. I'd like to avoid that if possible. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/#review1277 --- On 2011-04-25 03:04:12, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/ --- (Updated 2011-04-25 03:04:12) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ISA parser: Allow defining operand types with a ctype directly. Diffs - src/arch/isa_parser.py de679a068dd8 Diff: http://reviews.m5sim.org/r/656/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.
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: Allow defining operand types with a ctype directly.
On 2011-05-28 22:04:00, Steve Reinhardt wrote: This looks fine to me, but I'm confused... don't you delete this code completely two patches from now? Why bother changing it if you're going to get rid of it? Gabe Black wrote: Because this way both are available, and the ISAs can be converted one at a time and everything will work between every patch. Otherwise I'd have to break everything that wasn't (or was) updated, or do everything in one big change that does too much at once. Once all the ISAs are consistently on the new system, the old system isn't needed anymore and is deleted in that later patch. Steve Reinhardt wrote: OK, I see. Overall this seems like overkill; I can see how this code was useful while you were developing, but for committing, one complete patch that gets rid of the old way of doing things and fixes all the ISAs to use the new way would be preferable to me. I think it would be less confusing because all the related changes would be right there in one place. I don't have a problem with large changesets (in terms of lines of code or files touched), just ones that are not conceptually unified, and spreading this change across multiple csets goes against conceptually unified in the opposite direction, IMO. Gabe Black wrote: That makes sense, but then I think when changes get to be too big, they get to be too hard to understand all at once. By keeping each part relatively small it makes things easier to digest later. My enormous PC state change is an example of the opposite extreme, and it was probably a lot of work to get through and review and overwhelming to look at later in the history. I'd like to avoid that if possible. Then again, the changes after this one don't add much complexity to this one since they're pretty simple. Merging them is reasonable, and I'll do that once we're ready to put these in. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/#review1277 --- On 2011-04-25 03:04:12, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/ --- (Updated 2011-04-25 03:04:12) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ISA parser: Allow defining operand types with a ctype directly. Diffs - src/arch/isa_parser.py de679a068dd8 Diff: http://reviews.m5sim.org/r/656/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: Allow defining operand types with a ctype directly.
On 2011-05-28 22:04:00, Steve Reinhardt wrote: This looks fine to me, but I'm confused... don't you delete this code completely two patches from now? Why bother changing it if you're going to get rid of it? Gabe Black wrote: Because this way both are available, and the ISAs can be converted one at a time and everything will work between every patch. Otherwise I'd have to break everything that wasn't (or was) updated, or do everything in one big change that does too much at once. Once all the ISAs are consistently on the new system, the old system isn't needed anymore and is deleted in that later patch. Steve Reinhardt wrote: OK, I see. Overall this seems like overkill; I can see how this code was useful while you were developing, but for committing, one complete patch that gets rid of the old way of doing things and fixes all the ISAs to use the new way would be preferable to me. I think it would be less confusing because all the related changes would be right there in one place. I don't have a problem with large changesets (in terms of lines of code or files touched), just ones that are not conceptually unified, and spreading this change across multiple csets goes against conceptually unified in the opposite direction, IMO. Gabe Black wrote: That makes sense, but then I think when changes get to be too big, they get to be too hard to understand all at once. By keeping each part relatively small it makes things easier to digest later. My enormous PC state change is an example of the opposite extreme, and it was probably a lot of work to get through and review and overwhelming to look at later in the history. I'd like to avoid that if possible. Gabe Black wrote: Then again, the changes after this one don't add much complexity to this one since they're pretty simple. Merging them is reasonable, and I'll do that once we're ready to put these in. Yea, that's what I was thinking... while patches can get pretty big, if you just combine the ISA-specific operand_type fixes (like http://reviews.m5sim.org/r/657, but basically doing the same thing to all the ISAs) with the one that permanently changes how the operand_type block is processed (http://reviews.m5sim.org/r/658, which basically makes this one irrelevant), then it's still not that big. I'm definitely not suggesting that you fold in http://reviews.m5sim.org/r/655, just that you combine this one (656) with 657 ( siblings) and 658. - Steve --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/#review1277 --- On 2011-04-25 03:04:12, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/ --- (Updated 2011-04-25 03:04:12) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ISA parser: Allow defining operand types with a ctype directly. Diffs - src/arch/isa_parser.py de679a068dd8 Diff: http://reviews.m5sim.org/r/656/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.
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: garnet: add network ptr to links
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/711/#review1289 --- Ship it! - Brad On 2011-05-23 18:37:49, Tushar Krishna wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/711/ --- (Updated 2011-05-23 18:37:49) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, Nathan Binkert, and Brad Beckmann. Summary --- garnet: added network ptr to links to be used by orion Diffs - src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc 3f37cc5d25bc src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.hh 3f37cc5d25bc src/mem/ruby/network/orion/NetworkPower.cc 3f37cc5d25bc Diff: http://reviews.m5sim.org/r/711/diff Testing --- Thanks, Tushar ___ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request: orion: bug fix in link power, and some reorg
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/712/#review1290 --- Ship it! - Brad On 2011-05-23 18:40:06, Tushar Krishna wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/712/ --- (Updated 2011-05-23 18:40:06) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, Nathan Binkert, and Brad Beckmann. Summary --- orion: bug fix in link power, and some reorg Diffs - src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc 3f37cc5d25bc src/mem/ruby/network/garnet/fixed-pipeline/Router_d.hh 3f37cc5d25bc src/mem/ruby/network/orion/NetworkPower.cc 3f37cc5d25bc Diff: http://reviews.m5sim.org/r/712/diff Testing --- Thanks, Tushar ___ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
Re: [gem5-dev] Review Request: Ruby: Convert to M5 Stats
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/704/#review1291 --- I apologize it took me a while to review this code. Overall, my major concern is that the patch removes some of the most valuable stats in Ruby. Specific comments below: src/mem/ruby/profiler/Profiler.hh http://reviews.m5sim.org/r/704/#comment1754 Please, do not delete these stats. These stats are very useful in discovering performance bottlenecks in protocols. As far as those protocols checked into the tree, I believe the hammer protocol uses most, if not all of these stats. The term wCC stands for with Cache Coherence and signifies those requests that require cache-to-cache transfers. src/mem/ruby/profiler/Profiler.cc http://reviews.m5sim.org/r/704/#comment1755 This code was used to monitor cache port and bank contention when Ruby modeled those resources. I'm pretty sure that no current protocols enable the bank resource feature, but if the port resource is approximated by the number of transitions per cycle. It is possible for that port resource to cause problems and this stat would catch it. Thus I would like to keep these stats. src/mem/ruby/profiler/Profiler.cc http://reviews.m5sim.org/r/704/#comment1756 Yes, Korey your last observation is exactly why those histograms are initialized with a fixed bin size. It makes comparisons between different runs possible. I assume you can do the same thing with M5 stats, correct? src/mem/ruby/profiler/Profiler.cc http://reviews.m5sim.org/r/704/#comment1757 Did you move this logic somewhere else, or did you determine that none of the checked in protocols use this function? src/mem/ruby/profiler/Profiler.cc http://reviews.m5sim.org/r/704/#comment1758 Please keep this funcitonality. It is very useful to understand the behavior of the system. - Brad On 2011-05-16 15:06:16, Derek Hower wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/704/ --- (Updated 2011-05-16 15:06:16) Review request for Default, Nathan Binkert, Korey Sewell, and Brad Beckmann. Summary --- This patch contains changes to convert Ruby's stat handling to the M5-style Stat class. The ultimate goal is to remove Profiler entirely, though this patch only represents a (significant) step towards that goal. Some stats remain in the Profiler, notably those that do not have an obvious object to hold them (like Address Profiler) or those that I'm not sure what they do (e.g., *wCC*). Also, this patch does not contain a Garnet stats conversion (though the simple network is converted). Diffs - src/mem/ruby/network/simple/SimpleNetwork.hh UNKNOWN src/mem/ruby/network/simple/SimpleNetwork.cc UNKNOWN src/mem/ruby/network/simple/Throttle.hh UNKNOWN src/mem/ruby/network/simple/Throttle.cc UNKNOWN src/mem/ruby/profiler/AddressProfiler.hh UNKNOWN src/mem/ruby/profiler/AddressProfiler.cc UNKNOWN src/mem/ruby/profiler/CacheProfiler.hh UNKNOWN src/mem/ruby/profiler/CacheProfiler.cc UNKNOWN src/mem/ruby/profiler/MemCntrlProfiler.hh UNKNOWN src/mem/ruby/profiler/MemCntrlProfiler.cc UNKNOWN src/mem/ruby/profiler/Profiler.hh UNKNOWN src/mem/ruby/profiler/Profiler.cc UNKNOWN src/mem/ruby/slicc_interface/RubySlicc_Profiler_interface.hh UNKNOWN src/mem/ruby/slicc_interface/RubySlicc_Profiler_interface.cc UNKNOWN src/mem/ruby/system/CacheMemory.hh UNKNOWN src/mem/ruby/system/MemoryControl.hh UNKNOWN src/mem/ruby/system/Sequencer.hh UNKNOWN src/mem/ruby/system/Sequencer.cc UNKNOWN src/mem/ruby/system/System.cc UNKNOWN src/mem/slicc/symbols/StateMachine.py UNKNOWN Diff: http://reviews.m5sim.org/r/704/diff Testing --- Thanks, Derek ___ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
