This is an archive of the discontinued LLVM Phabricator instance.

[mips] bnec/beqc register constraint fix
ClosedPublic

Authored by sdardis on May 25 2016, 8:13 AM.

Details

Summary

beqc and bnec cannot have $rs == $rt. Inhibit compact branch creation
if that would occur.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 58421.May 25 2016, 8:13 AM
sdardis retitled this revision from to [mips] bnec/beqc register constraint fix.
sdardis updated this object.
sdardis added reviewers: dsanders, vkalintiris.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
dsanders requested changes to this revision.May 25 2016, 9:21 AM
dsanders edited edge metadata.

This needs a test case. Going by the original 7MB reproducer I think we need a test case with a conditional branch where both operands are evaluated in different basic blocks but can be CSE'd. I think the compiler should have eliminated the branch as well though since 'bnec $5, $5, ...' is never going to be taken.

This revision now requires changes to proceed.May 25 2016, 9:21 AM
sdardis updated this revision to Diff 58767.May 27 2016, 3:47 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.

Added a test reduced down from bug report.

dsanders requested changes to this revision.May 31 2016, 2:04 AM
dsanders edited edge metadata.
dsanders added inline comments.
test/CodeGen/Mips/compactbranches/beqc-bnec-register-constraint.ll
35–36 ↗(On Diff #58767)
; CHECK-NOT: bnec $[[R0:[0-9]+]], R0

The R0 and R1 should be [[R0]] and [[R1]]

Also, I'm not sure variables can be used in the same directive that defines them. I'll double check that.

This revision now requires changes to proceed.May 31 2016, 2:04 AM
dsanders accepted this revision.May 31 2016, 2:10 AM
dsanders edited edge metadata.

LGTM with the missing [[ and ]] added.

test/CodeGen/Mips/compactbranches/beqc-bnec-register-constraint.ll
35–36 ↗(On Diff #58767)

Also, I'm not sure variables can be used in the same directive that defines them. I'll double check that.

This works.

This revision is now accepted and ready to land.May 31 2016, 2:10 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review.