[PATCH] D46794: [mips] Add support for isBranchOffsetInRange and use it for MipsLongBranch

[Simon Dardis via Phabricator via llvm-commits]

sdardis added inline comments.


================
Comment at: lib/Target/Mips/MipsInstrInfo.cpp:370-392
+  case Mips::BC1EQZC_MMR6:
+  case Mips::BC1NEZC_MMR6:
+  case Mips::BC2EQZC_MMR6:
+  case Mips::BC2NEZC_MMR6:
+  case Mips::BEQC_MMR6:
+  case Mips::BNEC_MMR6:
+  case Mips::BGEC_MMR6:
----------------
abeserminji wrote:
> Is this offset correct here?
> I find in documentation that offset is 16 bits for these instructions, and can't figure out why is it compared here to 20 bits.
Yes, that's an error.

BC(1|2)(EQ|NE)ZC, B{...}ZALC, B((N|O)VC have 17 bit offsets,  the rest have 18 bit offsets.


================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:588
-      int ShVal = STI.inMicroMipsMode() ? 2 : 4;
-      int64_t Offset = computeOffset(I->Br) / ShVal;
 
----------------
abeserminji wrote:
> If this part is removed, then the below estimation for the NaCl will be estimated several times more.
Yes, but we're checking a wider / different range.

The old logic is testing if the encoded offset (i.e. we have right shifted the value) would fit in a 16 bit offset in the instruction.

The new logic is testing if the offset in bytes is in range of the branch instructions without the right shift.

|Offset:|           32| 32|
|After shift step:|  8| NA|
|NaCl adjustment:|  16| 64|
|Test:|      isInt<16>| isInt<18>|



Repository:
  rL LLVM

https://reviews.llvm.org/D46794