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

Fixups: Implement sign extension + Fixes #65

Merged
merged 6 commits into from
Jan 5, 2019

Conversation

stffrdhrn
Copy link
Member

Working on fixing up some of the test cases in or1k-tests. I found l.ext* instructions were never implemented. I implemented it using case because I thought its easy to read like that. But if others have suggestions let me know.

@stffrdhrn
Copy link
Member Author

This looks to fix #53

@stffrdhrn
Copy link
Member Author

I added a commit the this PR to fix #48 too.

@olofk
Copy link
Member

olofk commented Dec 30, 2018

The core file and the shorter fixes look fine to me. I'm afraid I don't know the code well enough to give a proper review of the ext stuff, but it looks like it does the job. Only thing I'm wondering is if this will put more logic in the alu_result_o path even when FEATURE_EXT is disabled? Please take a look at that, but I'm happy to see these patches merged anway

@stffrdhrn
Copy link
Member Author

stffrdhrn commented Dec 31, 2018

Thanks for the review. It's a good point in the extra logic on alu_result_o. I tried to model it like the other optional features which I assumed would be optimized out when disabled. I will check what I can see in the netlist, I might find I need to fix other optional functions as well.

Copy link
Contributor

@bandvig bandvig left a comment

Choose a reason for hiding this comment

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

I think you shiuld add round brackets for decode_op_ext_o (in mor1kx_decode) expression in the way:
assign decode_op_ext_o = opc_insn == OR1K_OPCODE_ALU && (opc_alu == OR1K_ALU_OPC_EXTBH ||
opc_alu == `OR1K_ALU_OPC_EXTW);

Taken from the latest fusesoc-cores
Reported by Alexander that he was seeing XXX's on the flag on init.
Adding a reset seems to work.
This has been defined as a feature but never implemented.  Tested with
or1k-test on iverilog.
Found in or1k-tests `or1k-alignillegalinsn`.  When jumping to a bad
address like 0xff000000 the instruction bus raises an error.  At this
point the fetch unit is waiting for the cache fill to be full and never
checks for error causing a lockup.

This change will allow us to detect an error and stop the cache
refill.
This allows us to document it and also control it during builds.
@stffrdhrn
Copy link
Member Author

stffrdhrn commented Jan 2, 2019

Thank you, I made the change to clean up decode_op_ext_o logic as suggested by Andrey and this should also help to remove the logic from alu_result_o when extension is disabled. But I am away from my desktop and don't know how to check the synthesis results from the command line.

Also, one strange thing, I am trying to test the mor1kx build with de0_nano-multicore. But Its failing to fit. It seems something has come into mor1kx or another core which is causing us to go over the resource limit now. I am looking into it. Once its fixed I will merge this.

Also, I added one more commit to pull the BRANCH PREDICTION parameter up to the mor1kx top.

@stffrdhrn stffrdhrn changed the title Fixups: Implement sign extension + Reset LSU full flag Fixups: Implement sign extension + Fixes Jan 2, 2019
@stffrdhrn
Copy link
Member Author

This also fixes #66 now, after reverting the true_pram change from @imphil. Philipp, let me know if you know if there is a better solution.

Introduced with commit 73bf629 (true_dpram: Use a single sequential block).

Change to use 2 clocks to better match what xilinx expects when
inferring true dual port RAM.  Tested and works on quartus/altera/intel.
@stffrdhrn
Copy link
Member Author

I updated the fix for #66 as discussed with Philipp, I am hoping switching to 2 clocks will work for xilinx, it works for altera.

@stffrdhrn
Copy link
Member Author

I am merging this. or1k-test results are looking OK.

Running or1k-alignillegalinsn                               PASS
Running or1k-backtoback_jmp                                 PASS
Running or1k-basic                                          PASS
Running or1k-cmov                                           PASS
Running or1k-cy                                             FAIL  - failing as mor1kx doesn't support multiply carry
Running or1k-dsx                                            PASS
Running or1k-dsxinsn                                        PASS
Running or1k-ext                                            PASS
Running or1k-ffl1                                           PASS
Running or1k-icache                                         PASS
Running or1k-illegalinsn                                    PASS
Running or1k-illegalinsndelayslot                           PASS
Running or1k-insnfetchalign                                 PASS
Running or1k-insnfetcherror                                 PASS
Running or1k-intloop                                        PASS
Running or1k-intmulticycle                                  PASS
Running or1k-intsyscall                                     PASS
Running or1k-inttickloop                                    PASS
Running or1k-jmp                                            PASS
Running or1k-jr                                             PASS
Running or1k-lsu                                            PASS
Running or1k-lsualign                                       PASS
Running or1k-lsualigndelayslot                              PASS
Running or1k-lsuerror                                       PASS
Running or1k-lsuerrordelayslot                              PASS
Running or1k-lwjr                                           PASS
Running or1k-msync                                          PASS
Running or1k-mul-basic                                      PASS
Running or1k-ov                                             PASS
Running or1k-regjmp                                         PASS
Running or1k-rfe                                            PASS
Running or1k-sf                                             PASS
Running or1k-sfbf                                           PASS
Running or1k-shiftopts                                      PASS
Running or1k-shortbranch                                    PASS
Running or1k-shortjump                                      PASS
Running or1k-systemcall                                     PASS
Running or1k-tickloop                                       PASS
Running or1k-tickrfforward                                  PASS
Running or1k-ticksyscall                                    PASS
Running or1k-timer                                          PASS
Running or1k-trap                                           PASS
Running or1k-trapdelayslot                                  PASS
Results                                                     Total:  43  FAIL:   1

Creating issue for multiply carry limitation.

@stffrdhrn stffrdhrn merged commit 6eefc64 into openrisc:master Jan 5, 2019
@juliusbaxter
Copy link
Member

juliusbaxter commented Jan 8, 2019

This all looks good to me. Nice work folks!

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.

4 participants