This is an archive of the discontinued LLVM Phabricator instance.

No *reserved* physical registers in the VirtRegMap
ClosedPublic

Authored by npjdesres on Jun 6 2017, 10:34 AM.

Details

Summary

(0) RegAllocPBQP: Since getRawAllocationOrder() may return a collection that includes reserved physical registers, iterate to find an un-reserved physical register.

(1) VirtRegMap: Enforce the invariant: "no reserved physical registers" in assignVirt2Phys(). Previously, this was checked only after the fact in VirtRegRewriter::rewrite.

(2) MachineVerifier: updated the test per MatzeB's review.

(3) +testcase

Diff Detail

Event Timeline

npjdesres created this revision.Jun 6 2017, 10:34 AM
MatzeB edited edge metadata.Jun 6 2017, 11:34 AM
  • The changes look fine to me (with an adjustment to the MachineVerifier change).
  • Patches should be uploaded with full context (see http://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface) to ease reviews next time.
  • Try to use clang-format to adhere to llvms coding conventions (see also nitpicks below)
  • I'd be fine committing the VirtRegMap/MachineVerifier fixes right away with the nitpicks addressed
  • We usually add a testcase for bugfixes like the PBQP one, is there any way to create a testcase for llvm trunk to check the PBQP problem? You probably need to find a target having a register class starting with a reserved register, .mir tests should allow it to just run regallocpbqp + virtregmap. (If there is no such targets then we are probably out of luck).
include/llvm/CodeGen/VirtRegMap.h
105

looking at this we could also change unsigned physReg to MCPhysReg PhysReg to make the intention even more obvious.

lib/CodeGen/MachineVerifier.cpp
1966

I think the actual bug here is a few lines earlier where it says if (!PVNI && LaneMask.none()). I think this should better read:

// All predecessors must have a live-out value. However for a phi instruction with subregister intervals
// only one of the subregisters (not necessarily the current one) needs to be defined.
if (!PVNI && (LaneMask.none() || !IsPhi)) { ... }
lib/CodeGen/RegAllocPBQP.cpp
742

We add a space after for, and if. Could also use: for (unsigned CandidateReg : RawPRegOrder) { ... }

lib/CodeGen/VirtRegMap.cpp
81

No space after (

I am addressing your comments. I might be able to make a mips testcase...

Perhaps you expected this, but running these files through clang-format makes for very large diffs.

MatzeB added a comment.Jun 6 2017, 2:07 PM

I am addressing your comments. I might be able to make a mips testcase...

Perhaps you expected this, but running these files through clang-format makes for very large diffs.

Yes llvm code certainly isn't clang-format clean and we don't tend to clean up sourcefiles after the fact.
However new code added/code changed should adhere to our style guidelines.

You can probably consider it more of an independent 3rd party you can refer to when criticizing style in reviews rather than something we mechanically pipe all of sourcecode through :)
There are scripts like clang/tools/clang-format/git-clang-format that apply clang format just to the lines changed in a file. I personally tend to just use clang-format as a reference/learning tool.

npjdesres updated this revision to Diff 101920.Jun 8 2017, 8:33 AM

Updated the patch to clang-format style guidelines. Updated the test in MachineVerifier.

I do not yet have a testcase. While trying to create a testcase for mips, I uncovered a different bug in the mips backend :-/ I'll submit a fix for the new bug shortly...

npjdesres updated this revision to Diff 101952.Jun 8 2017, 12:34 PM
npjdesres edited the summary of this revision. (Show Details)

Added testcase.

The testcase assert-fails in VirtRegRewriter::rewrite before this patch.

After this patch the testcase lowers successfully.

MatzeB accepted this revision.Jun 8 2017, 12:53 PM

LGTM

test/CodeGen/Generic/pbqp-reserved-physreg.ll
36–38

I assume the llvm.ident / !0 is not relevant for the test and can be removed.

This revision is now accepted and ready to land.Jun 8 2017, 12:53 PM
npjdesres updated this revision to Diff 101956.Jun 8 2017, 12:57 PM

Remove superfluous ident metadata from testcase.

MatzeB - thanks for the review. I don't have commit access. Would you mind? Thanks.

MatzeB added a comment.Jun 8 2017, 2:31 PM

Thanks! I moved the testcase to test/CodeGen/Mips (it would fail in the generic directory for people/bots that have the mips target disabled in their build configuration).

This revision was automatically updated to reflect the committed changes.
lhames added a subscriber: lhames.Jun 13 2017, 3:50 PM

Late to the party here, but this is great work - thanks!