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: 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: 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: Allow defining operand types with a ctype directly.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/656/#review1277 --- 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? - Steve 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
