-
Notifications
You must be signed in to change notification settings - Fork 34
First PR for vector changes to Testgen.py #580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First PR for vector changes to Testgen.py #580
Conversation
Comments at start of Python header |
Randomize GPR for address load |
…ing and is now saying it can't create a new process (fork: retry: Resource temporarily unavailable). So Was not able to look at the contents of the obj dump yet. Changes made were minimal though.
Looks like we now have a merge conflict. Otherwise, @jordancarlin have you had a chance to review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes. The more this grows the more it seems like testgen really needs a serious refactor. Considering the existing state of the script though, this looks good to me!
@@ -137,9 +133,16 @@ def writeSIGUPD_F(rd): | |||
# *** write this | |||
return "" | |||
|
|||
def writeSIGUPD_V(vd, sew): | |||
global sigOffset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a global variable called sigOffset
anymore. And it looks like you don't actually use it here anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this is still used, or at least shouldnt be delt with in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place I see it is in the unused incrementSigOffset
function. The rest of the code uses a different approach now.
if sew == 16: | ||
precision = 16 | ||
loadop = "flh" | ||
storeop = "sw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be sh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
def loadVecReg(reg, pointer, sew): | ||
loadop = f"vle{sew}.v" | ||
lines = "" | ||
addrGPR = randint(1,31) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need some way of avoiding the signature base register?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt the signature register move if it overlaps with the randomly generated register?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writeCovVector
calls handleSignaturePointerConflict
to work around this. Looks like this uses writeCovVector_V
which doesn't call this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is causing and issue. Working on fixing it
loadop = "fld" | ||
storeop = "sd" | ||
lines = lines + "la x2, scratch\n" | ||
lines = lines + f"li x3, {formatstrFP.format(val)} # load x3 with value {formatstrFP.format(val)}\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a chance these hardcoded registers could conflict with the signature register.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same convention is followed in function "loadFloatReg"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadFloatReg
(and all of the other floating point code) is not updated to work with signatures yet. Correct me if I'm wrong, but I thought the intention was to work signatures in from the start for vector.
Changes made on a new branch- the PR from yesterday had conflicts that I wasn't able to undo.
TODO: Small changes to testvector files for vlmax-dependent coverpoints in different length suites.
TODO: Change formatting of tests for some of the MVV.