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

CoreIR Dev merge #85

Closed
rdaly525 opened this issue Nov 3, 2018 · 16 comments
Closed

CoreIR Dev merge #85

rdaly525 opened this issue Nov 3, 2018 · 16 comments
Assignees

Comments

@rdaly525
Copy link
Contributor

rdaly525 commented Nov 3, 2018

Halide Changes: updated library file, fixed linebuffer issue
CoreIR Changes: Mostly changes with verilog generation
Mapper Changes: Fixes ASHR, and mux issues. Now only outputs configuration strings (instead of bits)
PnR/bitstream generator (TODO) correctly parse PE configuration from mapper.

The configuration strings should exactly conform to what is in the CGRAGenerator wiki for PE (as requested from Keyi)
@Kuree, @steveri, please update your repos to conform to this, and use the examples from the mapper dev branch to test these changes.

@Kuree
Copy link
Member

Kuree commented Nov 3, 2018

@rdaly525 I tested dev branch from CGRAMapper and it still has the mux bug. See: https://travis-ci.org/StanfordAHA/CGRAFlow/builds/450295178.

I looked through the commit log, it seems like muxfix branch is not merged onto dev.

@rdaly525
Copy link
Contributor Author

rdaly525 commented Nov 5, 2018

@Kuree Sorry, there was a mapper branch that was not merged into dev. It should work now. Although it does have a dependency on coreir dev branch.

@steveri
Copy link
Contributor

steveri commented Nov 5, 2018

If/when @Kuree tests complete successfully I think I'm good...

@Kuree
Copy link
Member

Kuree commented Nov 6, 2018

@rdaly525 @steveri It seems like something is broken in the flow, see: https://travis-ci.org/StanfordAHA/CGRAFlow/builds/451253598. The simulation result didn't match with the CPU reference.

I will find out which part is not functioning properly. The problem is I cannot reproduce the PnR result on my VM instance so it might take a while to locate the bug.

@leonardt
Copy link
Contributor

leonardt commented Nov 6, 2018

Why can't you reproduce the PnR result? Is it a non-determinism issue?

You could try dumping the PnR files from travis (easiest way would just be to cat it to stdout then copy it from the log).

You can also try running the travis job in debug mode. Since it's a public repo, you have to do it through the API, info here: https://docs.travis-ci.com/user/running-build-in-debug-mode/#restarting-a-job-in-debug-mode This gives you a job where you can SSH into the travis VM to poke around

@Kuree
Copy link
Member

Kuree commented Nov 6, 2018

My implementation only guarantees deterministic result for an environment. For instance, if you change Python version or g++, you will get different result. I haven't made any efforts to ensure it's consistent across all the platforms since it's deterministic if the environment is fixed.

I believe it's due different implementation on random and hash, but I'm not so sure.

The flow bug doesn't seem to relate to PnR since it passed all the checks. My educated guess would be one of the mapping/bsb is incorrect.

@Kuree
Copy link
Member

Kuree commented Nov 6, 2018

@steveri @leonardt Found a bug that's irrelevant to this issue:

  • There is a bug in tun_tbg.csh that ignores io_config because $io_config_override is never set. As a result, all the IO config information is controlled by 2in2out_cascade.json.
  • It is fine most of the time if cgra_pnr plays nice and produce IO pads that match with 2in2out_cascade.json.

I have fixed this bug and will send a PR soon.

@Kuree
Copy link
Member

Kuree commented Nov 7, 2018

@rdaly525 @steveri

There are several inconsistency between the new mapper and the downstream tools that caused the build failure. bsbuider doesn't take singed bit so cgra_pnr opts to ignore that. As a result, when producing bsb files, cgra_pnr directly uses alu_op string as the op code.

I've have branches pushed that fixed bugs mentioned in the thread. These branches need to merge into master.

@@ -47,22 +50,22 @@ python scripts/repo_manager.py
     --halide                      master                                        \
     --halide-remote               github.com/jeffsetter/Halide_CoreIR.git       \
                                                                                 \
-    --coreir                      master                                        \
+    --coreir                      dev                                           \
     --coreir-remote               github.com/rdaly525/coreir.git                \
                                                                                 \
     --pycoreir                    master                                        \
     --pycoreir-remote             github.com/leonardt/pycoreir.git              \
                                                                                 \
-    --mapper                      master                                        \
+    --mapper                      ashr                                          \
     --mapper-remote               github.com/StanfordAHA/CGRAMapper.git         \
                                                                                 \
-    --cgra-generator              master                                        \
+    --cgra-generator              fix_io_config                                 \
     --cgra-generator-remote       github.com/StanfordAHA/CGRAGenerator.git      \
                                                                                 \
     --test-bench-generator        master                                        \
     --test-bench-generator-remote github.com/StanfordAHA/TestBenchGenerator.git \
                                                                                 \
-    --cgra-pnr                    master                                        \
+    --cgra-pnr                    dev                                           \
     --cgra-pnr-remote             github.com/Kuree/cgra_pnr.git                 \

@steveri
Copy link
Contributor

steveri commented Nov 7, 2018

bsbuider doesn't take signed bit

It's supposed to work if you simply precede the opcode name with an optional 'u' or 's' ('u' is default)...examples:
Unsigned:

    T0x0306_sub.le(wire,const50__148)
    T0x0306_mul(wire,const50__148)
    T0x0705_eq(wire,const50__148)
    T0x1A09_lte_min.lt(wire,reg)
    T0x022F_gte(wire,reg)
    T0x0306_ugt(const16,wire)

Signed:

    T0x0306_ssub.le(wire,const50__148)
    T0x0306_smul(wire,const50__148)
    T0x0705_seq(wire,const50__148)
    T0x1A09_slte_min.lt(wire,reg)
    T0x022F_sgte(wire,reg)
    T0x0306_sgt(const16,wire)

@rdaly525
Copy link
Contributor Author

rdaly525 commented Nov 7, 2018

@steveri, can you update your bsbuilder to take as input a bool as to whether each op is signed? This is how it is described in the PE spec and we should consider that the truth and have our tools be consistent with it. For your human readable output, I think what you have above (prepending an 's') is fine.

@steveri
Copy link
Contributor

steveri commented Nov 7, 2018

haha what you call "human readable output" is actually the input to bsbuilder...it's an old-school assembler :)

if you want to be strictly formal with the input syntax then each op should be specified as follows:

[us]<official-op-name-from-pe-spec>.<official-flag-name-from-pe-spec>

i.e. you would not use the shorthand "T0x0306_mul(wire,const50__148)" (even though that's allowed); you would instead use the complete specification "T0x0306_umul.pe(wire,const50__148)"

TL;DR there is a bool it is the u/s prepended to the opcode name

@Kuree
Copy link
Member

Kuree commented Nov 7, 2018

Updated diff:

diff --git a/install.sh b/install.sh
index 283e432..496f39c 100755
--- a/install.sh
+++ b/install.sh
@@ -50,22 +50,22 @@ python scripts/repo_manager.py
     --halide                      master                                        \
     --halide-remote               github.com/jeffsetter/Halide_CoreIR.git       \
                                                                                 \
-    --coreir                      master                                        \
+    --coreir                      dev                                           \
     --coreir-remote               github.com/rdaly525/coreir.git                \
                                                                                 \
     --pycoreir                    master                                        \
     --pycoreir-remote             github.com/leonardt/pycoreir.git              \
                                                                                 \
-    --mapper                      master                                        \
+    --mapper                      dev                                           \
     --mapper-remote               github.com/StanfordAHA/CGRAMapper.git         \
                                                                                 \
-    --cgra-generator              master                                        \
+    --cgra-generator              fix_io_config                                 \
     --cgra-generator-remote       github.com/StanfordAHA/CGRAGenerator.git      \
                                                                                 \
     --test-bench-generator        master                                        \
     --test-bench-generator-remote github.com/StanfordAHA/TestBenchGenerator.git \
                                                                                 \
-    --cgra-pnr                    master                                        \
+    --cgra-pnr                    dev                                           \
     --cgra-pnr-remote             github.com/Kuree/cgra_pnr.git                 \
 

@rdaly525 would you please go ahead and merge coreir and mapper to master?
@steveri see https://github.com/StanfordAHA/CGRAGenerator/pull/137

I will merge cgra_pnr shortly.

@rdaly525
Copy link
Contributor Author

rdaly525 commented Nov 7, 2018

Keyi, presumably you generate the Steve-specific input format? Can you generate the way he wants? When we integrate this with garnet, these formats are going to need to be more explicitly defined and be as consistent as possible across tools. For now let’s just get it working.

@Kuree
Copy link
Member

Kuree commented Nov 7, 2018

For now I'm reading signed bit and using u/s to specifying either signed or unsigned. I agree that in future this has to be consistent and defined by garnet.

I will close this issue for now.

@Kuree Kuree closed this as completed Nov 7, 2018
@Kuree
Copy link
Member

Kuree commented Nov 7, 2018

@rdaly525 Ross can you merge mapper and coreir? The flow master branch is failing because I removed my fix script in cgra_pnr master branch.

@rdaly525
Copy link
Contributor Author

rdaly525 commented Nov 8, 2018

@Kuree Merged

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

No branches or pull requests

4 participants