This is an archive of the discontinued LLVM Phabricator instance.

RegAlloc: Fix "SubRange for this mask not found" unreachable in SplitKit
AbandonedPublic

Authored by arsenm on Jun 28 2021, 4:00 PM.

Details

Summary

In this testcase, %2 has some full defs and some subregister defs and
initially has two subranges for the low lane, and the rest of the full
register. The subregister def is not live out of the block where it
is defined.

After %2 is split, refineSubRanges ends up splitting the subrange
masks into a different set of masks than in the original parent
interval. This would then fail to find it and hit the unreachable.

I'm suspicious of how simple this patch is, and not sure why this
special phi handling is needed given that refineSubRanges was already
called Suspiciously, only one SystemZ test fails if I remove this
entire loop for subrange handling.

Diff Detail

Event Timeline

arsenm created this revision.Jun 28 2021, 4:00 PM
arsenm requested review of this revision.Jun 28 2021, 4:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 4:00 PM
Herald added a subscriber: wdng. · View Herald Transcript

I'm suspicious of how simple this patch is, and not sure why this special phi handling is needed given that refineSubRanges was already called

No sure. I am actually thinking that the new version is missing to update the ranges properly when the match is inexact (i.e., aren't we missing some updates?). Previously we had an expectation that the match was exact so the code was correct w.r.t. that simplification. Now, I am not so sure, but maybe like you said this job was actually already done in refineSubRanges.

@kparzysz what do you think?

Sorry, the emails that I'm getting from the @-mentions look exactly the same as any other email from phabricator to llvm-commits, and with hundreds of these per day I only look at a few...

The special handling of PHIs (with the exact mask) was added in D88020. It used to be finding a match that covered the given mask, but that commit made it look for an exact cover. It appears that it's essentially incompatible with the call to refineSubRanges.

Testcases for the more complex scenarios are accumulated as bug reproducers. There is no comprehensive coverage of all possible situations, so only one test failing with the removal of some piece of code is still pretty good (better than nothing).

foad added a comment.Aug 2 2021, 10:55 AM

The special handling of PHIs (with the exact mask) was added in D88020. It used to be finding a match that covered the given mask, but that commit made it look for an exact cover. It appears that it's essentially incompatible with the call to refineSubRanges.

That's not quite true. Before D88020 getSubRangeForMask was always "exact". D88020 split it into getSubRangeForMaskExact and getSubRangeForMask (which is the inexact version). So it should have been NFC in extendPHIRange and extendPHIKillRanges.

Ah, you're right, I didn't look closely enough.

In any case, seems like the problem originates with the refineSubRanges actually doing something non-trivial, as it would generally create subranges without direct matches in the PHI values. Maybe it has never worked for such cases. Frankly, I'm not sure what to do here, since the thing that comes to mind is to update the PHI nodes, but it would require function-wide refinement of the PHIs.

arsenm abandoned this revision.Oct 21 2021, 7:50 AM

Superseded by D107829