This is an archive of the discontinued LLVM Phabricator instance.

[Mips] Make MipsBranchExpansion::replaceBranch() aware of BBIT family of branch
ClosedPublic

Authored by djtodoro on Mar 26 2020, 6:22 AM.

Details

Summary

Octeon branches (bbit0/bbit032/bbit1/bbit132) have an immediate operand, so it is legal to have such replacement within MipsBranchExpansion::replaceBranch().

According to the specification, a branch (e.g. bbit0 ) looks like:

bbit0  rs p offset  // p is an immediate operand
  if !rs<p> then branch

Without this patch, an assertion triggers in the method, and the problem has been found in the real example.

Diff Detail

Event Timeline

djtodoro created this revision.Mar 26 2020, 6:22 AM
djtodoro edited the summary of this revision. (Show Details)Mar 26 2020, 8:10 AM

Two nits inlined. Otherwise this patch looks OK, eyeballing the supplied test.

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
345

This might be more cleanly expressed as a switch statement over the kind of operand, e.g.

switch (MO.getKind()) {
  case MachineOperand::MO_Register:
    MIB.addReg(MO.getReg();
    continue;
  case MachineOperand::MO_Immediate:
    if (!TII->isBranchWithImm(Br->getOpcode()))
      llvm_unreachable("Unexpected immediate in branch instruction!");
    MIB.addImm(MO.getImm());
    continue;
  case MachineOperand::MO_MachineBasicBlock:
    MIB.addMBB(MO);
    break;
  default:
    llvm_unreachable("Unknown operand kind in MipsBranchExpansion::replaceBranch(..)!");
}

break;

What do you think? You'll also move line 360 into the loop.

llvm/test/CodeGen/Mips/longbranch/long-branch-octeon.ll
3

Auto generate the checks with update_llc_checks.py.

Add a one line comment explaining the purpose of this test. E.g. "Test that Octeon bbit family of branch can be replaced with the long branch pass."

djtodoro marked 2 inline comments as done.Mar 27 2020, 2:44 AM

@sdardis Thanks for the review!

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
345

By moving the line 360 in the loop, that makes sense to me.

llvm/test/CodeGen/Mips/longbranch/long-branch-octeon.ll
3

Sure, I forgot to add it.

djtodoro updated this revision to Diff 253063.Mar 27 2020, 2:46 AM

-refactoring the code and the test
-addressing comments

atanasyan accepted this revision.Mar 27 2020, 1:38 PM

LGTM. Two nits inlined.

Do you have commit access?

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
349

Now we can use more convenient break here because there are no code inside the loop after the switch statement.

356

Ditto

This revision is now accepted and ready to land.Mar 27 2020, 1:38 PM
djtodoro marked an inline comment as done.Mar 27 2020, 1:52 PM

Thanks for the review!

Do you have commit access?

Yes, I do.

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
349

Sorry, I’m not sure I follow :) We need to collect all the operands, so we need to continue with the collecting it until we come to the MBB operand? If that is the case, we can’t use the break here.

atanasyan added inline comments.Mar 27 2020, 2:43 PM
llvm/lib/Target/Mips/MipsBranchExpansion.cpp
349

IMHO break in the switch statement just goes outside the switch. It doesn't stop the enveloping loop.

djtodoro marked an inline comment as done.Mar 30 2020, 12:41 AM
djtodoro added inline comments.
llvm/lib/Target/Mips/MipsBranchExpansion.cpp
349

Oh, I see.. never mind, I wasn't paying attention as much as I should.

Since I don't have a strong opinion on using break/continue within switch, I'll change it.
Thanks!

djtodoro updated this revision to Diff 253511.Mar 30 2020, 12:43 AM
djtodoro retitled this revision from [Mips] Make MipsBranchExpansion::replaceBranch() aware of bbit0/bbit032/bbit1/bbit132 to [Mips] Make MipsBranchExpansion::replaceBranch() aware of BBIT family of branch.

-addressing comments

This revision was automatically updated to reflect the committed changes.