Page MenuHomePhabricator

[X86] Make sure we copy the HandleSDNode back to N before executing the default code after the switch in matchAddressRecursively
ClosedPublic

Authored by craig.topper on Apr 17 2019, 3:45 PM.

Details

Summary

There are two places where we create a HandleSDNode in address matching in order to handle the case where N is changed by CSE. But if we end up not matching, we fall back to code at the bottom of the switch that really would like N to point to something that wasn't CSEd away. So we should make sure we copy the handle back to N on any paths that can reach that code.

This appears to be the true reason we needed to check DELETED_NODE in the negation matching. In pr32329.ll we had two subtracts back to back. We recursed through the first subtract, and onto the second subtract. The second subtract called matchAddressRecursively on its LHS which caused that subtract to CSE. We ultimately failed the match and ended up in the default code. But N was pointing at the old node that had been deleted, but the default code didn't know that and took it as the base register. Then we unwound back to the first subtract and tried to access this bogus base reg requiring the check for deleted node. With this patch we now use the CSE result as the base reg instead.

matchAdd has been broken since sometime in 2015 when it was pulled out of the switch into a helper function. The assignment to N at the end was still there, but N was passed by value and not by reference so the update didn't go anywhere.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 17 2019, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 3:45 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
niravd accepted this revision.Apr 18 2019, 6:48 AM

LGTM.

This revision is now accepted and ready to land.Apr 18 2019, 6:48 AM
This revision was automatically updated to reflect the committed changes.