> On 2011-02-24 14:56:09, Gabe Black wrote: > > This is actually better than my initial impression suggested. I think I saw > > all the lines from the Ra->URa change and got confused since those didn't > > have anything to do with RFE. I jumped to the conclusion that you guys had > > some big, elaborate, round about implementation for this when really you > > have two changes stuck together. > > > > I'm not 100% sure you're safe with out the PC change even if you rearrange > > things this way. What's going to happen is the NPC is going to be forced to > > PC + size of the instruction after every instruction is generated, even if > > that's wrong and is corrected by a branch mispredict. The problem manifests > > itself functionally when you have a mispredict and don't immediately move > > the NPC into the PC, preserving the correct control flow. That happened > > here because you were staying on the same inst to do more microops. I think > > most of the time if you put the branching microop last you'll get away with > > it, but I have a creeping doubt that there's some other corner case where > > it'll get you in trouble. I do agree that the ARM PC state is pretty > > complicated and it would be nice to slim it down somehow, or at least not > > make it worse.
There is always that case, but there is also the possibility that we'll accidentally introduce other weird bugs in the process. Right now we know this devil and there aren't many left. This is one of the last changes until the O3 cpu boots Linux/ARM. > On 2011-02-24 14:56:09, Gabe Black wrote: > > src/arch/arm/insts/mem.hh, line 111 > > <http://reviews.m5sim.org/r/474/diff/1/?file=10288#file10288line111> > > > > If these are fixed values you don't need to store them in the class. > > You can just define operands that always use those particular indexes. That > > said, it would probably be best to pass in indexes and make them fixed > > outside of this class. That makes it more obvious what values are being > > used and makes it easier to allow them to change in the future. ok. > On 2011-02-24 14:56:09, Gabe Black wrote: > > src/arch/arm/isa/operands.isa, line 231 > > <http://reviews.m5sim.org/r/474/diff/1/?file=10293#file10293line231> > > > > There isn't anything inherently wrong with changing these names, but it > > doesn't belong in this patch. It causes a lot of code to change that > > doesn't have anything to do with RFE. This patch made it clear that the names had to be changed, although it can be done in a separate patch. There was some conflict with one of the names that led to an invalid substitution. > On 2011-02-24 14:56:09, Gabe Black wrote: > > src/arch/arm/isa/operands.isa, line 236 > > <http://reviews.m5sim.org/r/474/diff/1/?file=10293#file10293line236> > > > > This one doesn't seem to actually be used. ok. - Ali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/474/#review894 ----------------------------------------------------------- On 2011-02-11 16:44:30, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/474/ > ----------------------------------------------------------- > > (Updated 2011-02-11 16:44:30) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > RFE changes control, so the last uop needs to do this otherwise the o3 thinks > there is a mispredict. > > > Diffs > ----- > > src/arch/arm/insts/macromem.hh 6548721032fa > src/arch/arm/insts/macromem.cc 6548721032fa > src/arch/arm/insts/mem.hh 6548721032fa > src/arch/arm/intregs.hh 6548721032fa > src/arch/arm/isa/insts/ldr.isa 6548721032fa > src/arch/arm/isa/insts/macromem.isa 6548721032fa > src/arch/arm/isa/insts/mem.isa 6548721032fa > src/arch/arm/isa/operands.isa 6548721032fa > src/arch/arm/isa/templates/macromem.isa 6548721032fa > src/arch/arm/isa/templates/mem.isa 6548721032fa > > Diff: http://reviews.m5sim.org/r/474/diff > > > Testing > ------- > > > Thanks, > > Ali > >
_______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
