----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/474/#review894 -----------------------------------------------------------
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. src/arch/arm/insts/mem.hh <http://reviews.m5sim.org/r/474/#comment1257> 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. src/arch/arm/isa/operands.isa <http://reviews.m5sim.org/r/474/#comment1258> 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. src/arch/arm/isa/operands.isa <http://reviews.m5sim.org/r/474/#comment1259> This one doesn't seem to actually be used. - Gabe 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 m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
