[PATCH] D48019: [mips] Handle branch expansion corner cases

[Simon Atanasyan via Phabricator via llvm-commits]

atanasyan added inline comments.


================
Comment at: lib/Target/Mips/MipsBranchExpansion.cpp:161
                      MachineBasicBlock *MBBOpnd);
+  bool BuildProperJumpMI(MachineBasicBlock *MBB, MachineBasicBlock::iterator Pos, DebugLoc DL);
   void expandToLongBranch(MBBInfo &Info);
----------------
- clang-format this long line
- start the function name with a small letter


================
Comment at: lib/Target/Mips/MipsBranchExpansion.cpp:364
 
+bool MipsBranchExpansion::BuildProperJumpMI(MachineBasicBlock *MBB, MachineBasicBlock::iterator Pos, DebugLoc DL) {
+  unsigned ATReg = ABI.IsN64() ? Mips::AT_64 : Mips::AT;
----------------
clang-format this function


================
Comment at: lib/Target/Mips/MipsBranchExpansion.cpp:368
+  bool addImm = false;
+  bool hasDelaySlot = false;
+
----------------
Start variable names with a capital letter.


================
Comment at: lib/Target/Mips/MipsBranchExpansion.cpp:392
+
+  return hasDelaySlot;
+}
----------------
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;
}
```


https://reviews.llvm.org/D48019