maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

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

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.

Reviewers: niravd, spatel, RKSimon, bkramer

Reviewed By: niravd

Subscribers: llvm-commits, hiraditya

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D60843

llvm-svn: 358735
This commit is contained in:
Craig Topper 2019-04-19 04:52:21 +00:00
parent 9a331bba2a
commit f73caae956
1 changed files with 7 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
View File

@ -210,7 +210,7 @@ namespace {
bool matchWrapper(SDValue N, X86ISelAddressMode &AM); bool matchWrapper(SDValue N, X86ISelAddressMode &AM);
bool matchAddress(SDValue N, X86ISelAddressMode &AM); bool matchAddress(SDValue N, X86ISelAddressMode &AM);
bool matchVectorAddress(SDValue N, X86ISelAddressMode &AM); bool matchVectorAddress(SDValue N, X86ISelAddressMode &AM);
bool matchAdd(SDValue N, X86ISelAddressMode &AM, unsigned Depth); bool matchAdd(SDValue &N, X86ISelAddressMode &AM, unsigned Depth);
bool matchAddressRecursively(SDValue N, X86ISelAddressMode &AM, bool matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
unsigned Depth); unsigned Depth);
bool matchAddressBase(SDValue N, X86ISelAddressMode &AM); bool matchAddressBase(SDValue N, X86ISelAddressMode &AM);
@ -1283,7 +1283,7 @@ bool X86DAGToDAGISel::matchAddress(SDValue N, X86ISelAddressMode &AM) {
return false; return false;
} }
bool X86DAGToDAGISel::matchAdd(SDValue N, X86ISelAddressMode &AM, bool X86DAGToDAGISel::matchAdd(SDValue &N, X86ISelAddressMode &AM,
unsigned Depth) { unsigned Depth) {
// Add an artificial use to this node so that we can keep track of // Add an artificial use to this node so that we can keep track of
// it if it gets CSE'd with a different node. // it if it gets CSE'd with a different node.
@ -1795,9 +1795,11 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
// Test if the LHS of the sub can be folded. // Test if the LHS of the sub can be folded.
X86ISelAddressMode Backup = AM; X86ISelAddressMode Backup = AM;
if (matchAddressRecursively(N.getOperand(0), AM, Depth+1)) { if (matchAddressRecursively(N.getOperand(0), AM, Depth+1)) {
N = Handle.getValue();
AM = Backup; AM = Backup;
break; break;
} }
N = Handle.getValue();
// Test if the index field is free for use. // Test if the index field is free for use.
if (AM.IndexReg.getNode() || AM.isRIPRelative()) { if (AM.IndexReg.getNode() || AM.isRIPRelative()) {
AM = Backup; AM = Backup;
@ -1805,7 +1807,7 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
} }
int Cost = 0; int Cost = 0;
SDValue RHS = Handle.getValue().getOperand(1); SDValue RHS = N.getOperand(1);
// If the RHS involves a register with multiple uses, this // If the RHS involves a register with multiple uses, this
// transformation incurs an extra mov, due to the neg instruction // transformation incurs an extra mov, due to the neg instruction
// clobbering its operand. // clobbering its operand.
@ -1818,9 +1820,7 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
++Cost; ++Cost;
// If the base is a register with multiple uses, this // If the base is a register with multiple uses, this
// transformation may save a mov. // transformation may save a mov.
// FIXME: Don't rely on DELETED_NODEs.
if ((AM.BaseType == X86ISelAddressMode::RegBase && AM.Base_Reg.getNode() && if ((AM.BaseType == X86ISelAddressMode::RegBase && AM.Base_Reg.getNode() &&
AM.Base_Reg->getOpcode() != ISD::DELETED_NODE &&
!AM.Base_Reg.getNode()->hasOneUse()) || !AM.Base_Reg.getNode()->hasOneUse()) ||
AM.BaseType == X86ISelAddressMode::FrameIndexBase) AM.BaseType == X86ISelAddressMode::FrameIndexBase)
--Cost; --Cost;
@ -1843,8 +1843,8 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
AM.Scale = 1; AM.Scale = 1;
// Insert the new nodes into the topological ordering. // Insert the new nodes into the topological ordering.
insertDAGNode(*CurDAG, Handle.getValue(), Zero); insertDAGNode(*CurDAG, N, Zero);
insertDAGNode(*CurDAG, Handle.getValue(), Neg); insertDAGNode(*CurDAG, N, Neg);
return false; return false;
} }