Page MenuHomePhabricator

[RegisterCoalescer] Add verification method to check LiveInterval Segments
Needs ReviewPublic

Authored by dstuttard on Nov 21 2017, 5:03 AM.

Details

Reviewers
qcolombet
MatzeB
Summary

I've been looking at SubRange Join asserts and this function is useful to help
pinpoint problems.

I've added it in this case as an optional post RegisterCoalescer verification
check that is controlled with the -verify-live-intervals command line flag.

In reality, this check is better done just after a call to joinCopy as this will
pinpoint the precise transformation that results in illegal Segments, but I
didn't want to add an additional check for a CL flag in the core of the pass.
If you are debugging this type of problem it is easier to manually insert the
call to the check where appropriate.

The verification pass determines if there are any Segments that contain a start
or end SlotIndex that no longer exist.
These types of issue are more common for SubRanges and most of the debugging
I've done has been in this area.

Event Timeline

dstuttard created this revision.Nov 21 2017, 5:03 AM

I've got a couple of new SubRange join fixes that I'm about to upload for review. This routine was useful in tracking those down so I thought it would be useful to add as a generic aid for future problems.

dstuttard updated this revision to Diff 123774.Nov 21 2017, 5:50 AM

Removed a blank line causing unnecessary differences

Removing debug code

qcolombet edited edge metadata.Nov 27 2017, 9:37 AM

Hi,

We already checks the liveinterval in the MachineVerifier (see MachineVerifier::checkLiveness). Could we share the logic between this one and the one in the MachineVerifier?

At the very least, the MachineVerifier should call this method.

Cheers,
-Quentin

MatzeB edited edge metadata.Nov 27 2017, 11:44 AM

Hi,

We already checks the liveinterval in the MachineVerifier (see MachineVerifier::checkLiveness). Could we share the logic between this one and the one in the MachineVerifier?

At the very least, the MachineVerifier should call this method.

Cheers,
-Quentin

+1 We should try to avoid duplicating all the code already present in the MachineVerifier. I'd rather see us starting to refactor the MachineVerifier code to be useful outside of the MachineVerifier.

Glancing at the code, would it be possible to extract some MachineVerifierBase class from MachineVerifier with foundErrors, the various reportXXX functions and verifyLiveIntervals()?

MatzeB added inline comments.Nov 27 2017, 11:45 AM
lib/CodeGen/RegisterCoalescer.cpp
98–101

Is there anything wrong with the existing -verify-coalescing switch that runs the machine verifier after coalescing, shouldn't that catch the same sort of errors?

The intent of this code is really to allow anyone doing debug to insert a check anywhere in the code. In particular, for the problems I was looking at I needed to check the live intervals after each change made in order to pinpoint where an error was being introduced. As implemented here (with the check after coalescing) it looks redundant, so having that extra check is probably useless.
Running a verification phase after coalescing is sometimes too late, and indeed may well have asserted before this point.

I agree that duplicating code isn't desirable, so the suggestion of re-factoring the existing MachineVerifier code to allow invocations in this way is probably better. I'll take a look.