Skip to content
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

Sail support for rvv #149

Closed
wants to merge 16 commits into from
Closed

Sail support for rvv #149

wants to merge 16 commits into from

Conversation

b224hisl
Copy link

@b224hisl b224hisl commented Feb 6, 2022

We are from RIOS lab, being responsible for providing sail support for RVV(RISCV-VECTOR).
This version of Sail-RVV codes have passed our hand-written assembly tests for LMUL=1, XLEN=64, VLEN=128 and VSEW=32. We will keep maintain this project and while generating more tests with other configs

@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 6, 2022

Commented-out code, random debugging script, random unwanted changes to existing code and documentation, committed binaries and even git conflict markers. I'm not reviewing this properly until you've actually reviewed it yourself for that kind of thing, because you clearly haven't; the dSYM file at least should have stood out like a sore thumb just looking at the list of files changed.

@martinberger
Copy link
Collaborator

martinberger commented Feb 6, 2022

Sail-RVV codes have passed our hand-written assembly tests

Thanks @b224hisl

Floating point code is notoriously hard to test. Is there a place where the tests can be seen easily, and where it is explained why you have chosen those specific tests? That would make reviewing easier.

Is it hard to wrap those tests into an easily reproducible script for reviewing purposes (doesn't have to go into the repo)?

@martinberger
Copy link
Collaborator

martinberger commented Feb 6, 2022

@b224hisl Can I gently suggest to consider closing this PR, while RIOS lab apply final polish to the code?

@allenjbaum
Copy link
Collaborator

allenjbaum commented Feb 7, 2022 via email

@martinberger
Copy link
Collaborator

@b224hisl New changes are still failing our CI.

@scottj97
Copy link
Contributor

scottj97 commented Feb 7, 2022

@b224hisl instead of repeatedly closing and reopening still-totally-broken Pull Requests, or quietly pushing still-totally-broken commits with cryptic messages like "delete sth", I suggest you (or someone else from your organization) discuss this here with the reviewers.

Otherwise, this is just spam.

Comment on lines 1 to 19
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleDevelopmentRegion</key>
<string>English</string>
<key>CFBundleIdentifier</key>
<string>com.apple.xcode.dsym.riscv_sim_RV32</string>
<key>CFBundleInfoDictionaryVersion</key>
<string>6.0</string>
<key>CFBundlePackageType</key>
<string>dSYM</string>
<key>CFBundleSignature</key>
<string>????</string>
<key>CFBundleShortVersionString</key>
<string>1.0</string>
<key>CFBundleVersion</key>
<string>1</string>
</dict>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This content is not edited by me, it is generated automatically

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add *.plist to your. gitignore to ensure that these don't get into the repo

Copy link
Contributor

@abukharmeh abukharmeh Mar 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a better idea to add it to $GIT_DIR/info/exclude instead (as that is specific to your workflow). gitignore is supposed to be common between all contributers, I am not sure how practical is it to add all various editors and IDEs to gitignore.

Copy link
Collaborator

@jrtc27 jrtc27 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*.plist is a bad idea, those can be source files. You want *.dSYM as a whole. Which really belongs in your machine's global gitignore file.

Comment on lines 1 to 20
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleDevelopmentRegion</key>
<string>English</string>
<key>CFBundleIdentifier</key>
<string>com.apple.xcode.dsym.riscv_sim_RV64</string>
<key>CFBundleInfoDictionaryVersion</key>
<string>6.0</string>
<key>CFBundlePackageType</key>
<string>dSYM</string>
<key>CFBundleSignature</key>
<string>????</string>
<key>CFBundleShortVersionString</key>
<string>1.0</string>
<key>CFBundleVersion</key>
<string>1</string>
</dict>
</plist>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete

debug_sail.sh Outdated
Comment on lines 2 to 19

function test_build () {
declare -i rc=0
eval $*
rc=$?
if [ $rc -ne 0 ]; then
echo "Failure to execute: $*"
exit $rc
fi
}

# test_build make ARCH=RV32 ocaml_emulator/riscv_ocaml_sim_RV32
# test_build make ARCH=RV64 ocaml_emulator/riscv_ocaml_sim_RV64

test_build make ARCH=RV32 c_emulator/riscv_sim_RV32 -j24
test_build make ARCH=RV64 c_emulator/riscv_sim_RV64 -j24
# test_build ./c_emulator/riscv_sim_RV64 ../riscv-tests-vector/isa/rv64uv-p-vsxseg >rv64uv-p-vsxseg.sail
# test_build ./c_emulator/riscv_sim_RV64 ../riscv-tests-vector/isa/rv64uv-v-vsxseg >rv64uv-v-vsxseg.b
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete ?

@@ -1,6 +1,6 @@
Extending the model
===================

ZHUYIFEI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert ?

@abukharmeh
Copy link
Contributor

abukharmeh commented Feb 7, 2022

Please note that I didnt finish looking at the files, I think you should, there is a lot of comments and files that need to be reverted before trying to review this. e.g binaries {riscv_sim_RV32, riscv_sim_RV64 and Z3 problems}, and IDE configurations (*plist). Sorry @jrtc27 for pointing out things you already mentioned, I just noticed that.

Comment on lines 13 to 17
test_build make ARCH=RV32 ocaml_emulator/riscv_ocaml_sim_RV32
test_build make ARCH=RV64 ocaml_emulator/riscv_ocaml_sim_RV64
#test_build make ARCH=RV32 ocaml_emulator/riscv_ocaml_sim_RV32
#test_build make ARCH=RV64 ocaml_emulator/riscv_ocaml_sim_RV64

test_build make ARCH=RV32 c_emulator/riscv_sim_RV32
test_build make ARCH=RV64 c_emulator/riscv_sim_RV64
test_build make ARCH=RV32 c_emulator/riscv_sim_RV32 -j24
test_build make ARCH=RV64 c_emulator/riscv_sim_RV64 -j24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert, Also if you have had issues with OCAML simulator, please let us know in #138 comments

@@ -35,7 +35,11 @@ let f64_mul rm v1 v2 =
let f64_div rm v1 v2 =
()

<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete merge process !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!I will handle the conflicts I didn't find before carefully.

Comment on lines 1 to 5
/* ****************************************************************** */
/* This file specifies the instructions in the vector set. */


/* ****************************************************************** */
Copy link
Contributor

@abukharmeh abukharmeh Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a suitable copyright header matching the rest of the repo as per #146

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there is no package called headache in CentOS, Could you please give me some advice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for headache, just copy the template from other files, and write a list of the contributors to each file added ??

@b224hisl
Copy link
Author

Sorry for replying late and thanks for your guide. I can pass the ./build_simulator and successfully build the simulator on my local end, but when I push my code to the repo, it can't pass the CI. I'm trying to find what's wrong
Besides, I do not have much experience in pulling request. I hope you will give me more guidance, I will get it done as soon as possible, Thank you very much!

@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 10, 2022

That script does not build the RVFI-DII variants of the model, which are what are failing to build

@martinberger
Copy link
Collaborator

martinberger commented Feb 10, 2022

successfully build the simulator on my local end, but when I push my code to the repo,

@b224hisl I imagine that you work locally against an older copy of the code, but while you work locally, the code in this repo changes, which causes the failures when you push to this repo. So I recommend regularly to pull from here to your local repo and merge any changed that we get here back into your local repo. while you test locally. In particular, do this just before you submit code here.

In general, git is a powerful tool, and it takes a while to become familiar with even a fraction of the features. I spent two week (= 4 lectures) teaching git when I taught software engineering. In particular diffing, branching, merging and rebasing is important.

@martinberger
Copy link
Collaborator

@b224hisl Could it be that you are accidentally push to the official repo, when you want to push locally?

@b224hisl
Copy link
Author

b224hisl commented Feb 12, 2022

Hello, I have another question. I can make rvfi successfully on my local end, but it can't pass the CI, it will report bugs like this:

make: *** [generated_definitions/c/riscv_rvfi_model_RV32.c] Killed
make: *** Deleting file 'generated_definitions/c/riscv_rvfi_model_RV32.c'
make: *** Waiting for unfinished jobs....
Makefile:291: recipe for target 'generated_definitions/c/riscv_rvfi_model_RV32.c' failed

So it will automatically kill this building. I don't know whether the building time matters. Could anyone help me with that? Besides, I do have merged the lastest codes of the sail-riscv and handled all conflicts, which may not be the cause of the problem

@abukharmeh
Copy link
Contributor

abukharmeh commented Feb 12, 2022

I just tried to compile it, and it does indeed compile fine, though memory usage of the SAIL compiler peak at more than 7 GB, while compiling the master branch takes a bit more than 1GB on my Debian machine.

I think the vector code is making the SAIL compiler use a lot of memory, and that results in GH killing the process around the 7GB mark matching public information I found about GH runners RAM capacity.

We need to find why the vector code makes the SAIL compiler use that much more memory!

@abukharmeh
Copy link
Contributor

abukharmeh commented Feb 12, 2022

I think easiest way to debug this would be to split the vector instructions into multiple files and see if we can narrow it down to a given instruction or couple of instructions that are resulting in a significant increase in RAM usage.

Maybe that can be even automated with csplit and some bash script that monitor RAM usage across compilations
csplit riscv_insts_vext_total.sail '/mapping clause encdec.*/' {10} --prefix=riscv_inst_vext --suffix="%02d.sail"

I am not sure if it would be easy to pint point what is happening with the SAIL compiler with valgrind !

EDIT: I wonder if this is a systematic issue that need addressing at the compiler level, perhaps its worth trying to compile with P and V together and monitor RAM usage !

@martinberger
Copy link
Collaborator

martinberger commented Feb 12, 2022

monitor RAM usage !

@abukharmeh I doubt we are already running into Github Action resource limits, but if resources are an issue, we should try and get more. Compute is cheap.

@abukharmeh
Copy link
Contributor

Thats also an option, according to this 7GB is the GitHub action RAM capacity for Linux machines, Apparently, GitHub can be connected to an external runner, maybe that's what we should do then! I cannot see anything in the log that indicates any error, Sail compiler is just getting killed, and on my local machine, it definitely took more than 7GB to compile the model with Vector extension!

//print_int("i=",i)
}
};
//print("end loop")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug comments perhaps should be removed before trying to merge with master ?

Comment on lines 131 to 133
//result[i] = sail_sign_extend(0b1, vsew_bits)
result[i] = vd_val[i];
//print("agnositc")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug comments perhaps should be removed before trying to merge with master ?

else if tail_ag == AGNOSTIC then {
//result[i] = sail_sign_extend(0b1, vsew_bits)
result[i] = vd_val[i];
//print("agnostic0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug comments perhaps should be removed before trying to merge with master ?

@@ -225,6 +226,7 @@ char *process_args(int argc, char **argv)
"N"
"I"
"F"
"W"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment !

Comment on lines +1 to +43
import sys
import os
import math

vlen = int(sys.argv[1])
elen = int(sys.argv[2])
sail_path = 'model/riscv_vlen.sail'

# Check value of VLEN
if vlen < 128:
vlen = 128
print('WARN: VLEN less than 128; setting VLEN to 128\n')
if vlen % 2 != 0:
vlen = vlen - (vlen % 2)
print('WARN: VLEN not a power of 2; setting VLEN to %d\n' % vlen)

# Check value of ELEN
if elen < 8:
elen = 8
print('WARN: ELEN less than 8; setting ELEN to 8\n')
if elen % 2 != 0:
elen = elen - (elen % 2)
print('WARN: ELEN not a power of 2; setting ELEN to %d\n' % elen)

if os.path.exists(sail_path):
os.remove(sail_path)

lb = ['/* Define the VLEN value for the architecture */\n',
'\n',
'type vlen : Int = %d\n' % vlen,
'type vlen_bytes : Int = %d\n' % (vlen / 8),
'type vstart_int : Int = %d\n' % math.log(vlen, 2),
'type vstartbits = bits(vstart_int)\n',
'\n',
'type elen : Int = %d\n' % elen,
'\n']


fh_sail = open('model/riscv_vlen.sail', 'w')
fh_sail.writelines(lb)
fh_sail.close()

exit()
Copy link
Contributor

@abukharmeh abukharmeh Mar 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This opens the same discussion about configurability and the idea of auto-generating SAIL code. While this script is trivial, and one can clearly reason its inputs and outputs, I wonder if it's better to have these as options to the simulators as cmd line args instead of generating them like this, Is there anything that prevents us from assigning these at runtime?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think this is not needed, if we cannot set these at runtime for whatever reason, then people can just change them in that file, perhaps we should be checking that VLEN and ELEN actually make sense inside the model rather than here ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are going to prohibit any dynamic change of the Sail model. The Sail model is a constant, and the C-model it generates is a constant. Any behavioral change will be parameters that are passed to the Csim as commandline arguments (some of them rather complex, which is what we are trying to pin downs, e.g. lists of lists - but these are simple)

var real_num_elem : int = undefined;
if lmul >= 1.0 then {
real_num_elem = num_elem;
//print("init_masked_result: lmul >= 1.0")
Copy link
Contributor

@abukharmeh abukharmeh Mar 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug info like this need to be removed before merging ?

If there exist any debug information that we think would be very useful at a later stage, then we perhaps should have them guarded by a higher verbosity debug print?

I don't think the model has variable verbosity debug logging at the moment, but perhaps that's something someone could argue that is needed ? @billmcspadden-riscv

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these kind of "debug droppings" should be removed or, if they are potentially useful, then a verbosity level should be added.

The SystemVerilog UVM package has a pattern we could follow. Is that something that people would respect and use?

Comment on lines +9 to +23
# Check value of VLEN
if vlen < 128:
vlen = 128
print('WARN: VLEN less than 128; setting VLEN to 128\n')
if vlen % 2 != 0:
vlen = vlen - (vlen % 2)
print('WARN: VLEN not a power of 2; setting VLEN to %d\n' % vlen)

# Check value of ELEN
if elen < 8:
elen = 8
print('WARN: ELEN less than 8; setting ELEN to 8\n')
if elen % 2 != 0:
elen = elen - (elen % 2)
print('WARN: ELEN not a power of 2; setting ELEN to %d\n' % elen)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values of parameters from the caller should not be considered suggestions, which you can ignore if you don't like their values. They should be considered commands. If the code cannot obey those commands, it should fail immediately with a reasonable error message, instead of doing something other than what was commanded.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Mar 7, 2022 via email

@b224hisl
Copy link
Author

b224hisl commented May 20, 2022

Hello, Thanks for your advice. And we are fixing the bugs of this sail-rvv project and deleting the unrelated information.
As memtioned above, there exists the problem of running out of the RAM so we can't pass the CI, though we can compile the csim and rvfi on the local end. Is there any feedback to this problem?
Besides, when buliding the osim, it will report "too many non-constant constructor“(#138 ). We have followed the guide in rems-project/sail#165, using another verison of sail. But it still can't be successfully compiled even without the V-extension

@b224hisl
Copy link
Author

Could someone help me handle the questions I mentioned above?

@martinberger
Copy link
Collaborator

Could you give us some more information regarding what fails when you compile without the V extension? What are compiling in this case and what is the error message you are getting? What are you compiling when you run out of RAM using GitHub actions?

@b224hisl
Copy link
Author

Could you give us some more information regarding what fails when you compile without the V extension? What are compiling in this case and what is the error message you are getting? What are you compiling when you run out of RAM using GitHub actions?

when I try to bulid the osim, it will report "too many non-constant constructor", I have upload my error log in #138 (comment).

And running out of the github RAM is another thing. When I try to bulid the other two simulators, I can successfully bulid them on my local end, but it can not pass the CI here.

I just tried to compile it, and it does indeed compile fine, though memory usage of the SAIL compiler peak at more than 7 GB, while compiling the master branch takes a bit more than 1GB on my Debian machine.

I think the vector code is making the SAIL compiler use a lot of memory, and that results in GH killing the process around the 7GB mark matching public information I found about GH runners RAM capacity.

We need to find why the vector code makes the SAIL compiler use that much more memory!

@bacam
Copy link
Collaborator

bacam commented Jun 27, 2022

I looked into this today. In the runner currently used in this pull request it runs two copies of sail in parallel, so the increase in memory usage is doubled. The master branch now has a different runner that appears to only build one at a time, so it will probably fit in memory. You should be able to use it by rebasing the branch.

The osim issue should be fixed shortly when Alasdair releases a new Sail version, but that will require a few small changes, all of the same form. I'll add an example.

Comment on lines 6728 to 8290
union clause ast = NITYPE : (nifunct6, bits(1), regidx, regidx, regidx)

mapp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mixed assignment / definition using a tuple won't be supported by the next Sail version. The result_real line isn't necessary anyway and can be removed. Instead put a let in front of the tuple to make it a definition rather than an assignment.

Copy link
Collaborator

@bacam bacam Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, github has changed the line range to a different piece of code entirely. Here's an example patch instead:

--- a/model/riscv_insts_vext_total.sail
+++ b/model/riscv_insts_vext_total.sail
@@ -8285,9 +8285,8 @@ function process_rfvv_widen(funct6, vm, vs2, vs1, vd, num_elem, vsew_bits, lmul)
   vs1_0 : bits('double_vsew) = read_single_element(double_vsew, 0, double_lmul, vs1);
   foreach (i from 0 to (num_elem - 1)) {
     if mask_helper[i] == true then {
-      result_real : real = undefined;
       /* Currently Ordered/Unordered sum do the same operations */
-      (fptype, sign, result_real) = fpreal_add(fp_to_real(vs2_val[i]), fp_to_real(vs1_0));
+      let (fptype, sign, result_real) = fpreal_add(fp_to_real(vs2_val[i]), fp_to_real(vs1_0));
       vs1_0 = real_to_fp(result_real)
     }
   };

@xiak95
Copy link

xiak95 commented Dec 28, 2022

This is currently being superseded with #191 #198 etc, so can be closed ?

@billmcspadden-riscv
Copy link
Collaborator

billmcspadden-riscv commented Jan 2, 2023 via email

@github-actions
Copy link

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit b69d069. ± Comparison against base commit 5cc5a93.

@jjscheel jjscheel mentioned this pull request Mar 17, 2023
13 tasks
@ptomsich
Copy link
Collaborator

Looking at the history, we apparently had an agreement that this PR can be closed, as it is superseded by #191, #198, and others.

@ptomsich ptomsich closed this May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.