> Looking further, it seems that there has been confusion on NumIntRegs
> v. NumArchIntRegs on Jaidev's implementation.
Can we talk to Jaidev? Do you have his e-mail address? I'd like to
get this figured out. Do they have updated code perhaps?
> Wrgpr and Rdgpr are functions for the sole purpose of having shadow
> set register compatibility.
It seems that these instructions are the only one that use those
functions which seems decidedly odd.
> Those functions need to take whatever the source register value and
> then use a control register to compute which shadow set to get to. The
> shadow set needs to be computed by getting the offset and adding it to
> the source register value.
>
> So in short, change that NumIntReg to NumArchIntRegs in both the rdgpr
> and wrgpr instructions and we should be fine.
Can you look at readReg and setReg as well? They have big problems as
well. There is a multiplier by NumIntRegs in there too. I can change
that to NumIntArchRegs, but then the functions still don't make too
much sense. I've copied the code here and I'll comment on what I have
issues with in readReg, setReg on the other hand has issues, but
they're different. Why for example aren't these functions symmetric?
> IntReg
> IntRegFile::readReg(int intReg)
> {
> if (intReg < NumIntRegs) {
Ok, we've agreed that this check just goes
> // Regular GPR Read
> DPRINTF(MipsPRA, "Reading Reg: %d, CurrShadowSet: %d\n", intReg,
> currShadowSet);
>
> if (intReg >= NumIntArchRegs * NumShadowRegSets) {
Is this condition correct? Should we just be checking if intReg >=
NumIntArchRegs?
> return regs[intReg + NumIntRegs * currShadowSet];
Assuming the existing condition, intreg is already greater than
NumIntArchRegs * NumShadowReg sets. If we then add intReg to anything
multipled by currShadowSet, you'll go out of bounds (even if we change
NumIntRegs to NumIntArchRegs.)
Should this line be:
return regs[intReg + NumIntRegs * NumShadowRegSets];
> } else {
> int index = intReg + NumIntArchRegs * currShadowSet;
> index = index % NumIntArchRegs;
> return regs[index];
What's with the modulo? This code forces the index to be between 0
and NumIntArchRegs, meaning that it will be in shadow set 0. Why then
are we multiplying by the currShadowSet?
If we were to change the condition the way I suggested above and we
delete the modulo, then if you're trying to access a regular register,
it will cause you to access the currShadowSet
> }
> } else {
> // Read from shadow GPR .. probably called by RDPGPR
> return regs[intReg];
> }
This else clause goes away.
> }
> In terms of testing the result, well, I'm not sure every single
> instruction gets stressed in the MIPS Linux boot. This implementation
> was to get every single instruction in the MIPS ISA 32 specification
> as "MIPS-compliant" as possible but there isnt a full suite of test
> cases that I can pull from. The tests that were ran to verify this
> code were MIPS in-house regressions so I couldnt possibly get those.
Are you sure that they wouldn't give any of them away?
> What has to happen is that the other regressions we have just need to
> be compiled for MIPS and we should be good to go. Things have changed
> in O3 so I cant say for sure it would work given how delay slots must
> work for MIPS in O3. However, it's kind of a time-sync in terms of
> "what's the next thing to do" because as well as the regressions for
> MIPS, there is also the inorder model, O3 compatibility and there is
> also inserting a booting MIPS Linux code into the tree.
The problem is, if there are no MIPS regressions, problems like this
are going to cause MIPS to bitrot and die. As we improve things, we'd
like to improve MIPS along with it, but if we can't test our changes,
we won't always know what to do.
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
