This is an archive of the discontinued LLVM Phabricator instance.

[mips] Merge MipsLongBranch and MipsHazardSchedule passes
ClosedPublic

Authored by abeserminji on May 9 2018, 8:14 AM.

Details

Summary

MipsLongBranchPass and MipsHazardSchedule passes are joined to one pass because of mutual conflict.
When MipsHazardSchedule inserts 'nop's, it potentially breaks some jumps, so they have to be expanded
to long branches. When some branch is expanded to long branch, it potentially creates a hazard situation,
which should be fixed by adding nops.
This pass now combine these two passes, and runs them alternately until one of them reports no changes
were made.

Diff Detail

Event Timeline

abeserminji created this revision.May 9 2018, 8:14 AM
abeserminji added inline comments.May 10 2018, 2:18 AM
lib/Target/Mips/MipsBranchExpansion.cpp
788

I thought one more time about this, and I believe that this loop and it's condition are wrong. We have to repeat these steps until no branches are expanded, as branch expansion can cause another branch to go out of range. With loop like that, this situation is possible:

  • expand branch
  • handle forbidden slots
  • exit loop

which may leave some branches broken.
I will fix that in the next update.

sdardis added inline comments.May 14 2018, 5:05 AM
lib/Target/Mips/MipsBranchExpansion.cpp
14

it inserts nops to prevent forbidden slot hazards.

16

combines these two tasks

20

a forbidden slot hazard

36

bc, b (pseudo) instructions

109

mips-branch-expansion

abeserminji marked 6 inline comments as done.

Comments resolved.
Regarding my inline comment, there is no need to update that, as that case is already covered in handlePossibleLongBranch(...) function in while(...) loop.
Removed unused part of code, related to MBB address calculation.

sdardis added inline comments.May 15 2018, 7:56 AM
lib/Target/Mips/MipsBranchExpansion.cpp
142

"Mips Branch Expansion Pass"

369–372

These two can be lifted to be private class members, so all functions can access them.

709–712

This hunk should be part of the runOnMachineFunction function, rather than here.

lib/Target/Mips/MipsTargetMachine.cpp
296–301

Rework this comment to explain what the pass does rather than making references to MipsLongBranchPass and MipsHazardSchedule.

test/CodeGen/Mips/branch-relaxation-with-hazard.ll
2–3

Use -mtriple=mips-img-linux-gnu here and use the update_llc_checks.py script on this file.

5

Remove this.

abeserminji marked 6 inline comments as done.

Comments addressed.

abeserminji added inline comments.May 16 2018, 6:21 AM
test/CodeGen/Mips/branch-relaxation-with-hazard.ll
2–3

I did as suggested, though I am not sure if this is the expected output.
There are no checks after #APP for both cases.

LGTM after addressing my comments inline.

lib/Target/Mips/MipsBranchExpansion.cpp
775

This isn't clang formatted.

test/CodeGen/Mips/branch-relaxation-with-hazard.ll
2–3

I see that now. It's to do with inline assembly getting wrapped in .set directives that correspond to the assembly that ends a function. You'll have to go back to your handwritten test instead.

You'll also want to update the title.

sdardis accepted this revision.May 17 2018, 8:14 AM
This revision is now accepted and ready to land.May 17 2018, 8:14 AM
abeserminji marked 3 inline comments as done.
abeserminji retitled this revision from [mips] WIP: Merge MipsLongBranch and MipsHazardSchedule passes to [mips] Merge MipsLongBranch and MipsHazardSchedule passes.

Comments addressed.
Regressions fixed.

This revision was automatically updated to reflect the committed changes.

Updated the patch, so that buildbots do not fail.
The problem was in forcing the extension to long branches in every iteration,
which causes infinite loop.

lib/Target/Mips/MipsBranchExpansion.cpp