This is an archive of the discontinued LLVM Phabricator instance.

[InlineSpiller]Re-tie operands if folding failed
ClosedPublic

Authored by skatkov on Jan 11 2021, 12:44 AM.

Details

Summary

InlineSpiller::foldMemoryOperand unties registers before an attempt to fold and
does not restore tied-ness in case of failure.

I do not have a particular test for demo of invalid behavior. This is something of clean-up.
It is better to keep the behavior correct in case some time in future it happens,

Diff Detail

Event Timeline

skatkov created this revision.Jan 11 2021, 12:44 AM
skatkov requested review of this revision.Jan 11 2021, 12:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 12:44 AM
dantrushin accepted this revision.Jan 11 2021, 7:56 AM

LGTM

llvm/lib/CodeGen/InlineSpiller.cpp
855–856

We can move this below LoadMI && MO.isDef() check.

This revision is now accepted and ready to land.Jan 11 2021, 7:56 AM

I would strongly prefer that instead of simply asserting that we retie the registers. Doing so should be fairly simple - we just need to keep track of which register pairs we untied for the possible fold, and use a lambda to retie them.

Despite that, I am also LGTMing this as an interim step.

Context for those reading along, there's some form of register allocator related miscompile happening in a complicated statepoint example. This bit of code came up in an audit for possible issues - thus the desire to rule it out with asserts. Reducing the example down to a small enough test case to analyze has been challenging.

skatkov updated this revision to Diff 315987.Jan 11 2021, 8:43 PM
skatkov retitled this revision from [InlineSpiller] Add an assert for untied registers to [InlineSpiller]Re-tie operands if folding failed.
skatkov edited the summary of this revision. (Show Details)

Ok, I modified the patch to re-tie operands in case of folding failure.

skatkov requested review of this revision.Jan 11 2021, 8:43 PM
This revision is now accepted and ready to land.Jan 12 2021, 4:48 AM

Also LGTM. Thanks.

This revision was automatically updated to reflect the committed changes.