This is an archive of the discontinued LLVM Phabricator instance.

[mips] Enforce compact branch restrictions
ClosedPublic

Authored by sdardis on Aug 16 2016, 4:36 AM.

Details

Summary

Check both operands for use of the $zero register which cannot be used with
an compact branch instruction.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 68159.Aug 16 2016, 4:36 AM
sdardis retitled this revision from to [mips] Enforce compact branch restrictions.
sdardis updated this object.
sdardis added reviewers: dsanders, vkalintiris.
sdardis added a subscriber: llvm-commits.
dsanders requested changes to this revision.Aug 16 2016, 6:03 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/MipsInstrInfo.cpp
437–442

There are currently six iterations over the operand list. 5 are findRegisterUseOperandIdx() calls (two of which are inside readsRegister()), then one more to copy the operands. findRegisterUseOperandIdx() checks subregisters so we can cut it down to two with:

int ZeroOperandPosition = -1;
if (I->isBranch() && !I->isPseudo()) {
  // Check for reads of any portion of ZERO_64.
  ZeroOperandPosition = I->findRegisterUseOperandIdx(Mips::ZERO_64);
  BranchWithZeroOperand = ZeroOperandPosition != -1;
}

then add the necessary signed -> unsigned cast in the loop below.

483–484

indentation

test/CodeGen/Mips/compactbranches/no-beqzc-bnezc.ll
106–107

Could you check for the correct branch instruction instead? It's easy to get false-passes with just CHECK-NOT.

121–122

Could you check for the correct branch instruction instead?

This revision now requires changes to proceed.Aug 16 2016, 6:03 AM
sdardis updated this revision to Diff 68178.Aug 16 2016, 8:09 AM
sdardis edited edge metadata.

Updated as per reviewer's comments.

dsanders accepted this revision.Aug 16 2016, 8:19 AM
dsanders edited edge metadata.

LGTM with a couple nits

lib/Target/Mips/MipsInstrInfo.cpp
438

I think the 'auto' should be 'const auto &' if possible and 'auto &' otherwise.

439

I think we should include a comment that this will also find ZERO_64

This revision is now accepted and ready to land.Aug 16 2016, 8:19 AM
sdardis closed this revision.Aug 16 2016, 10:25 AM

Thanks, committed rL278824.