This is an archive of the discontinued LLVM Phabricator instance.

[mips] Add support for isBranchOffsetInRange and use it for MipsLongBranch
ClosedPublic

Authored by sdardis on May 12 2018, 10:56 AM.

Details

Summary

Add support for this target hook, covering MIPS, microMIPS and MIPSR6, along
with some tests. Also add missing getOppositeBranchOpc() cases exposed by the
tests.

Diff Detail

Event Timeline

sdardis created this revision.May 12 2018, 10:56 AM
abeserminji added inline comments.May 14 2018, 4:55 AM
lib/Target/Mips/MipsInstrInfo.cpp
370–392

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.

abeserminji added inline comments.May 14 2018, 4:58 AM
lib/Target/Mips/MipsLongBranch.cpp
588

If this part is removed, then the below estimation for the NaCl will be estimated several times more.

sdardis added inline comments.May 14 2018, 7:58 AM
lib/Target/Mips/MipsInstrInfo.cpp
370–392

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.

lib/Target/Mips/MipsLongBranch.cpp
588

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:3232
After shift step:8NA
NaCl adjustment:1664
Test:isInt<16>isInt<18>
abeserminji added inline comments.May 14 2018, 8:39 AM
lib/Target/Mips/MipsLongBranch.cpp
588

Oh yes, it's true. I didn't connect these things immediately.
Tnx for the explanation.

sdardis updated this revision to Diff 146630.May 14 2018, 9:21 AM

Address comments.

This revision is now accepted and ready to land.May 15 2018, 6:39 AM
This revision was automatically updated to reflect the committed changes.