> On 2011-06-13 07:27:06, Korey Sewell wrote: > > src/cpu/inorder/resources/bpred_unit.cc, line 377 > > <http://reviews.m5sim.org/r/744/diff/1/?file=13011#file13011line377> > > > > Can you post a separate patch for the MipsISA stuff you are describing? > > I dont see anything in the DIFF. Also, can you list benchmarks that you > > have observed to work? > > > > My overall comment is that I think I caught #s1-3 in the new InOrderCPU > > code (the code to get Full System working). > > > > It's just stuck behind the patches on the reviewboard being merged and > > passing the regressions. It's predominantly done now, w/the DTB patch being > > closed to resolved. The last hiccup seems to be that the 50.vortex > > regressions seems to have been broken in the merge. Once that gets > > resolved, we'll get a wave of inorder updates. > > > > At that point (in the near future), we'll compare the branch code and > > I'm hoping that you (Deyuan Guo) would be OK retesting your branch changes. > > Deyuan Guo wrote: > diff -r 9fb150de362e src/arch/mips/isa/formats/branch.isa > --- a/src/arch/mips/isa/formats/branch.isa Sun Jun 12 23:52:21 > 2011 -0700 > +++ b/src/arch/mips/isa/formats/branch.isa Tue Jun 14 14:13:47 > 2011 +0800 > @@ -234,7 +234,7 @@ > if x == 'Link': > code += 'R31 = NNPC;\n' > elif x == 'Likely': > - not_taken_code = 'NNPC = NPC; NPC = PC;' > + # not_taken_code = 'NNPC = NPC; NPC = PC;' > inst_flags += ('IsCondDelaySlot', ) > else: > inst_flags += (x, ) > @@ -273,7 +273,7 @@ > if x == 'Link': > code += 'R32 = NNPC;' > elif x == 'Likely': > - not_taken_code = 'NNPC = NPC, NPC = PC;' > + # not_taken_code = 'NNPC = NPC, NPC = PC;' > inst_flags += ('IsCondDelaySlot', ) > else: > inst_flags += (x, ) > > > Korey Sewell wrote: > For future reference, this is best placed in a separate patch on the > reviewboard, but this seems ok since it's a small diff. > > I'm curious as to why this doesn't work? In a Conditional delay slot, if > you're branch isn't taken, then the next PC should be the NNPC right?... > As in taken: > NPC = PC+4 (sizeof instruction) > NNPC = target > > And not taken: > NPC = PC+8 > NNPC = PC+12 > > ARe you seeing that the not taken code is generating a duplicate > instruction or ??? > > Deyuan Guo wrote: > Thanks for you reply. > In my opinion, according to 'NPC=PC+8;NNPC=PC+12', the not_taken_code > should be 'NPC=NNPC;NNPC+=4;'. > But then I find that there is no difference when I comment the 2 lines or > not. > The not_taken_code will be added to > build/MIPS_SE/arch/mips/inorder_cpu_exec.cc, under branch likely > instructions, but it seems that it won't affect the executing procedure? > > ---- > We are using dhrystone benchmarks for fixed-point testing, compiled by a > MIPS64 compiler, which generates many instructions such as beql/ld/sd/lld etc. > During the work, we fixed several bugs of inorder cpu model, and commit > the diff here. > But we only test MIPS64 in SE mode, and there are still some problems > about the float-point part, and lld/LLSC instructions. > Btw, is there any plan to support MIPS64 in gem5? > > Korey Sewell wrote: > For the Branch Likely Code, are you sure that your compiler is generating > those type of instructions? Commenting the code out doesn't seem like an > actual solution but more of a avoidance of a bug. If it is a bug, then we > should just fix it. > ---- > From the internal gem5 development team, I dont know of any active MIPS64 > development. I had done some work in the past and followed it up a couple > years ago, but that's not what I'm working. > > I have heard a few questions about MIPS at the tutorial and through the > list recently. Since gem5 is such a community-like project, there are > typically a number of people 'silently' working on a particular extension > (e.g. MIPS64) or wanting that same feature and willing to put in work to > develop (as long as the burden of development isn't overly taxing) > > I'd suggest if you are interested in MIPS64, send out an inquiry to the > gem5-dev list asking who else is interested and also list the features that > aren't implemented currently but that you need for it to work. At the very > minimum, the gem5-dev list can give you advice on where to start your > development but in the ideal case someone else will want the same feature and > you can team up on development. > > Korey Sewell wrote: > In the future, you can try updating your current changeset (you can do > this w/the reviewboard command line: hg postreview -e -u <#> -o). That will > save you having to post a new patch and delete the old one. > > Deyuan Guo wrote: > Thanks for your advice. I'll try to use hg postreview next time. The > latest updated inordercpu codes are more robust. The bug of linked load > instructions I mentioned above is disappearing now.
Great, I'm glad thinks are working better. Can you delete this patch review (discard) and then update the changes on the last review you added? Also, is there a particular benchmark that exposes the branch likely functionality better? If so, we would like to include that in the regressions. In general, we need more MIPS testing, so if you have time, it'd be great if you could add even a few MIPS regressions (all it takes is adding 4 files to the regression directory) and it would help you in that all future changes to gem5 would test against MIPS functionality. - Korey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/744/#review1328 ----------------------------------------------------------- On 2011-06-13 01:11:53, Deyuan Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/744/ > ----------------------------------------------------------- > > (Updated 2011-06-13 01:11:53) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > We are porting gem5 to support MIPS64, and find some problems. We try to fix > them, as shown below: > > 1. fetch_seq_unit.cc > Comment 2 lines. pc shouldn't be changed at this condition; > 2. branch_predictor.cc > Comment 1 line. By default, set target to NPC is OK. > Adding a condition for MIPS branch likely instruction. > 3. bpred_unit.cc > If the branch is not taken, BTB should not be updated. > > In addition, 2 places in src/arch/mips/isa/formats/branch.isa: > elif x == 'Likely': > # not_taken_code = 'NNPC = NPC, NPC = PC;' > The not_taken_code should be commented. > > > Diffs > ----- > > src/cpu/inorder/resources/bpred_unit.cc 9fb150de362e > src/cpu/inorder/resources/branch_predictor.cc 9fb150de362e > src/cpu/inorder/resources/fetch_seq_unit.cc 9fb150de362e > > Diff: http://reviews.m5sim.org/r/744/diff > > > Testing > ------- > > We have run many benchmarks of MIPS64 in gem5. > And for MIPS32, with CPU_MODELS=InOrderCPU, hello world run correctly. > > > Thanks, > > Deyuan > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
