This is an archive of the discontinued LLVM Phabricator instance.

[mips] Handle branch expansion corner cases
ClosedPublic

Authored by abeserminji on Jun 11 2018, 5:53 AM.

Details

Summary

With this patch, it is calculated if potential jump instruction and target are in the same segment.
If so, jump instruction with immediate field is used.

Also, branch expansion does not cover cases where offset does not fit immediate value of a bc/j instructions.
With this patch, offset is stored into registers, and then jump register instruction is used.

Diff Detail

Event Timeline

abeserminji created this revision.Jun 11 2018, 5:53 AM
sdardis added inline comments.Jun 11 2018, 8:05 AM
lib/Target/Mips/MipsBranchExpansion.cpp
636

Use isBranchOffsetInRange rather than isInt<28>. It does the same thing but performs information hiding.

699–704

Separate out this into a private function of this class and replace all the occurrences of similar logic with a call to the new function.

713

This nop is unneeded for MIPSR6 when using jrc or bc.

786

These variables are unused.

abeserminji marked 4 inline comments as done.

Comments addressed.

sdardis added inline comments.Jun 13 2018, 6:59 AM
lib/Target/Mips/MipsBranchExpansion.cpp
363

Sorry, better idea here.

Instead, turn this into a wrapper around BuildMI, taking the first 3 arguments of BuildMI.

Compute the relevant opcode for all possible variants including microMIPS, Call out to BuildMI with the computed opcode adding the right AT/AT_64 register, then conditionally add the immediate of zero for R6 if required.

Then return true/false if the inserted instruction has a delay slot.

483–510

This can all be refactored then. Insert the branch instruction with the extended helper. If the inserted instruction has no delay slot or the target is NaCl, put the stack adjustment before the inserted instruction. Otherwise insert the nop/stack adjust as appropriate after the jump and bundle it with the branch.

601–617

Similarly here. This code is missing the NaCl changes.

706–715

The tail of the N64 case and the tail of the N32/O32 case where the jump instruction is inserted here can be merged here with the extended helper, conditionally bundling the NOP if required.

abeserminji marked 4 inline comments as done.

Comments addressed.

atanasyan added inline comments.Jul 4 2018, 4:22 AM
lib/Target/Mips/MipsBranchExpansion.cpp
161
  • clang-format this long line
  • start the function name with a small letter
363

clang-format this function

367

Start variable names with a capital letter.

391

It looks like addImm and hasDelaySlot always have different values. Maybe use, for example, the first one and return from the function !addImm?

BTW what do you think about the code below? I think it's more clear represent selection of the jump operation. But I do not force this variant.

bool MipsBranchExpansion::buildProperJumpMI(MachineBasicBlock *MBB,
                                            MachineBasicBlock::iterator Pos,
                                            DebugLoc DL) {
  bool HasR6 = ABI.IsN64() ? STI->hasMips64r6() : STI->hasMips32r6();
  bool AddImm = HasR6 && !STI->useIndirectJumpsHazard();

  unsigned JR = ABI.IsN64() ? Mips::JR64 : Mips::JR;
  unsigned JIC = ABI.IsN64() ? Mips::JIC64 : Mips::JIC;
  unsigned JR_HB = ABI.IsN64() ? Mips::JR_HB64 : Mips::JR_HB;
  unsigned JR_HB_R6 = ABI.IsN64() ? Mips::JR_HB64_R6 : Mips::JR_HB_R6;

  unsigned JumpOp;
  if (STI->useIndirectJumpsHazard())
    JumpOp = HasR6 ? JR_HB_R6 : JR_HB;
  else
    JumpOp = HasR6 ? JIC : JR;

  if (JumpOp == Mips::JIC && STI->inMicroMipsMode())
    JumpOp = Mips::JIC_MMR6;

  unsigned ATReg = ABI.IsN64() ? Mips::AT_64 : Mips::AT;
  MachineInstrBuilder Instr =
      BuildMI(*MBB, Pos, DL, TII->get(JumpOp)).addReg(ATReg);
  if (AddImm)
    Instr.addImm(0);

  return !AddImm;
}
abeserminji marked 3 inline comments as done.

Comments addressed.

abeserminji added inline comments.Jul 18 2018, 12:18 AM
lib/Target/Mips/MipsBranchExpansion.cpp
391

I've checked your solution and it looks nicer to me. So I am gonna include it.

This revision is now accepted and ready to land.Jul 19 2018, 6:09 AM
This revision was automatically updated to reflect the committed changes.