mips: add support for branch likely instruction

Information
Submitter:	Deyuan Guo
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

Make the newly gem5 support mips branch likely instruction again.
Fix 4 files:
src/arch/mips/isa/formats/branch.isa
src/cpu/inorder/inorder_dyn_inst.cc
src/cpu/inorder/resources/branch_predictor.cc
src/cpu/inorder/resources/fetch_seq_unit.cc

Testing Done

Tested.

src/arch/mips/isa/formats/branch.isa (Diff revision 1)

Great, thanks for the quick code review and change.

src/cpu/inorder/inorder_dyn_inst.cc (Diff revision 1)

In general, 
commenting out code in patches is not a good practice as it leads to lots of old text floating through code. 

If the old code has some remaining purpose, then we should place a comment there for future reference.

Otherwise, the best thing to do is just delete it. 

Can you update this to either delete that comment or place a note why that code is still there?

Assuming this patch has been tested for the MIPS regressions we have (and you have), this looks good. 

Lastly, I want to test on a SPARC hello world regression that I have.

src/cpu/inorder/resources/branch_predictor.cc (Diff revision 1)

I'd like to avoid "#if ISA_HAS_DELAY_SLOT" as much as possible in code that's not in src/arch/*, but this may be a spot where it's necessary

src/cpu/inorder/resources/fetch_seq_unit.cc (Diff revision 1)

same as above.

src/cpu/inorder/resources/fetch_seq_unit.cc (Diff revision 1)

This will work for all architectures right without the "#if"? 

No other defined ISAs mark that CondDelaySlot() flag.

I dont think saving the check here will be worth the clutter. Can you remove then test some alpha regressions? Maybe just the hello world-quick-alpha regression. and the 70.twolf-long-alpha regression?

src/cpu/inorder/resources/fetch_seq_unit.cc (Diff revision 1)

same as above. Checking for that cond-delay-slot flag already separates out the MIPS code. We can get rid of the #if and possibly put a comment below (or beside that line w/in 80 characters) saying that the 2nd condition handles MIPS branch likely instructions.

Change Summary:

o. Delete the unused code instead of commenting out it;
o. Remove the "#if ISA_HAS_DELAY_SLOT" because the isCondDelaySlot() is only used/set by MIPS ISA; 
o. Define readMiscRegNoEffect/setMiscRegNoEffect in inorder_dyn_inst.cc. Or else if we use this two functions in arch/mips/locked_mem.hh, ld will complain. (Sometimes, linked load instructions will make a thread halt then restart it. After that, the tick becomes a large number and it says 'because simulate() limit reached'. I haven't found the root cause yet. All I can do is using read/setMiscRegNoEffect instead of read/setMiscReg in arch/mips/locked_mem.hh...)

Diff:

Revision 2 (+21 -8)

Show changes

	src/arch/mips/isa/formats/branch.isa
	src/cpu/inorder/inorder_dyn_inst.cc
	src/cpu/inorder/resources/branch_predictor.cc
	src/cpu/inorder/resources/fetch_seq_unit.cc

Change Summary:

find a small bug: 
in src/arch/mips/isa/decoder.isa, nmadd/nmsub are wrongly defined.

Diff:

Revision 3 (+24 -11)

Show changes

	src/arch/mips/isa/decoder.isa
	src/arch/mips/isa/formats/branch.isa
	src/cpu/inorder/inorder_dyn_inst.cc
	src/cpu/inorder/resources/branch_predictor.cc
	src/cpu/inorder/resources/fetch_seq_unit.cc

src/arch/mips/isa/decoder.isa (Diff revision 3)

This is a separate patch then the mips branch prediction. IF you have updates for the same patch, then update it. 

If you are patching a differen issue, please post a new review for that.

This is important for logging of changesets.
Also, did you test this through observation of wrong output or through comparing against the ISA spec?

src/arch/mips/isa/decoder.isa (Diff revision 3)

same as above

Why would nmsub add and nmadd subtract?

Deyuan Guo June 20, 2011, 10:25 p.m. (June 20, 2011, 10:25 p.m.)

Actually, in the official MIPS ISA documents, the nmadd/nmsub means "multiply, add/subtract, then negtive".
In the earlier version, the code of nmsub_s is "Fd.sf = -1 * ((Fs.sf * Ft.sf) - Fr.sf);", but the brackets have been changed now.
I think someone have updated the code carelessly.

src/cpu/inorder/inorder_dyn_inst.cc (Diff revision 3)

separate patch.

also, what code in locked_mem.hh requires this?

Deyuan Guo June 20, 2011, 10:50 p.m. (June 20, 2011, 10:50 p.m.)

I have to use set/readMiscRegNoEffect instead of set/readMiscReg in the locked_mem.hh, to avoid a problem caused by link load instructions.(the tick becomes a large number and it says 'because simulate() limit reached'). 
But I haven't found the root cause, so I didn't commit a diff of locked_mem.hh.

Could you tell me, does a separate patch mean, delete the diff of nmadd/NoEffect here, and write a now review request?

src/arch/mips/isa/decoder.isa (Diff revision 3)

I see, the instruction was defined in a non-intuitive way.

Another way to write this (from the SEE MIPS RUN book is):
Fr.sf - (Fs.sf * Ft.sf) for nmsub_s
and
-1 * (Fs.sf * Ft.sf + Fr.sf) for nmadd_s

I agree w/your updates now. However, how did you find this error? Through inspection or running a benchmark? If the latter, we need to add that regression (or a similar one) to preserve that functionality.

Deyuan Guo June 20, 2011, 11:51 p.m. (June 20, 2011, 11:51 p.m.)

Well, we don't have a benchmark containing the two instructions. I just happened to see this by comparing the newly code with an earlier stable version..

src/cpu/inorder/inorder_dyn_inst.cc (Diff revision 3)

Separate patch means add a new review request. Eventually, I'll have to commit your patches to the tree (w/your email as author), so it's best not to merge nonrelated changes.

If you get "simulate() reached", that means your system likely deadlocked as there are no more events on the main event queue. Run w/the cpu progress interval on to help determine where the CPUs stops committing instructions. Also, post the locked_mem.hh patch if you want comments on what you are trying to do to resolve the problem.

Change Summary:

Move the unrelated patch to new review requests. Here are all about the MIPS branch likely instructions.

Diff:

Revision 4 (+9 -8)

Show changes

	src/arch/mips/isa/formats/branch.isa
	src/cpu/inorder/inorder_dyn_inst.cc
	src/cpu/inorder/resources/branch_predictor.cc
	src/cpu/inorder/resources/fetch_seq_unit.cc

src/arch/mips/isa/formats/branch.isa (Diff revision 4)

The last thing to do is pass the regressions.

Can you run the ALPHA quick regressions for inorder and also the MIPS quick regression and verify they pass?

Is there a dhrystone benchmark that exposes this branch likely functionality? It'd be nice to make sure we check that this does not get broken on future regression runs.

Deyuan Guo June 23, 2011, 1:10 p.m. (June 23, 2011, 1:10 p.m.)

Hello!
I just tested the following commands with the latest version:
  build/ALPHA_SE/m5.opt configs/example/se.py --inorder --caches
  build/MIPS_SE/m5.opt configs/example/se.py --inorder --caches
The helloworld tests of ALPHA and MIPS32 can run correctly. 
However, I don't know the meaning of "run the ALPHA quick regressions". Does it relate to the tests/quick folder? Could you tell me how to use this?

At last, I find that the branch likely instructions aren't generated by our MIPS compiler. These instructions are in our MIPS64 glibc libraries. So I think using an inline assembly testcase containing those instructions may be more efficiently than using the dhrystone benchmark or other C benchmarks.

Are there any updates to this patch?

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff: