[llvm] <Fix> incorrect IndexReg parameter (PR #82881)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-x86

Author: RicoAfoat (RicoAfoat)

<details>
<summary>Changes</summary>

See #<!-- -->82431 for more infomation.

Bug Trace:

`Zext` in the modified code piece might be replaced by other `SDValue` and deleted during `SelectInlineAsmMemoryOperand` method, but it remains as element in `Ops` vector which will be used to call `getNode` builder, which is illegal. 

```cpp
// llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
void SelectionDAGISel::Select_INLINEASM(SDNode *N) {
  SDLoc DL(N);

  std::vector<SDValue> Ops(N->op_begin(), N->op_end());
  SelectInlineAsmMemoryOperands(Ops, DL); // t39 is builded and deleted here, but it remains in Ops

  const EVT VTs[] = {MVT::Other, MVT::Glue};
  SDValue New = CurDAG->getNode(N->getOpcode(), DL, VTs, Ops);// t39 is a DELETED_NODE, assertion fails in builder
  New->setNodeId(-1);
  ReplaceUses(N, New.getNode());
  CurDAG->RemoveDeadNode(N);
}
```

```cpp
// llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
SDValue X86DAGToDAGISel::matchIndexRecursively(SDValue N,
                                               X86ISelAddressMode &AM,
                                               unsigned Depth) {
  ...
  if (Opc == ISD::ZERO_EXTEND && !VT.isVector() && N.hasOneUse()) {
    ...
    if (((SrcOpc == ISD::ADD && Src->getFlags().hasNoUnsignedWrap()) ||
         CurDAG->isADDLike(Src)) &&
        Src.hasOneUse()) {
      if (CurDAG->isBaseWithConstantOffset(Src)) {
        ...
        if (!foldOffsetIntoAddress(Offset * AM.Scale, AM)) {
          ...
          CurDAG->ReplaceAllUsesWith(N, ExtAdd); //t39 is replaced here, but later it will be used as 
          CurDAG->RemoveDeadNode(N.getNode());// parameters to call a builder method.
          return Res ? Res : ExtSrc;
        }
      }
    }
  }
 ...
  return N;
}
```

Reason Why Modify Like This:

1. `SelectInlineAsmMemoryOperands` is a method which all target machine architecture share, bug only happens in `x86`, can't be fixed here.
2. Due to the way `SelectInlineAsmMemoryOperand` interact with `SelectInlineAsmMemoryOperand`, it is hard to modify element in `Ops` after it is pushed into.
3. (In file `llvm/lib/Target/X86/X86ISelDAGToDAG.cpp`)`matchAddressRecursively` in case `ISD::ZERO_EXTEND` generates `Zext` and `NewShl`, but the generation is not consistent with how `matchAddressRecursively` deal with `ISD::SHL`. The latter calls `matchIndexRecursively` to replace `Zext`. I modified this so that the correct `AM.IndexReg` in this situation will enter `Ops`.

---
Full diff: https://github.com/llvm/llvm-project/pull/82881.diff


1 Files Affected:

- (modified) llvm/lib/Target/X86/X86ISelDAGToDAG.cpp (+5-4) 


``````````diff
diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index c8f80ced354538..50be61c56e951c 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -2732,13 +2732,14 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
       insertDAGNode(*CurDAG, N, Zext);
       SDValue NewShl = CurDAG->getNode(ISD::SHL, DL, VT, Zext, ShlAmt);
       insertDAGNode(*CurDAG, N, NewShl);
+      CurDAG->ReplaceAllUsesWith(N, NewShl);
+      CurDAG->RemoveDeadNode(N.getNode());
 
       // Convert the shift to scale factor.
       AM.Scale = 1 << ShAmtV;
-      AM.IndexReg = Zext;
-
-      CurDAG->ReplaceAllUsesWith(N, NewShl);
-      CurDAG->RemoveDeadNode(N.getNode());
+      // If matchIndexRecursively is not called here, 
+      // Zext may be replaced by other nodes but later used to call a builder method
+      AM.IndexReg = matchIndexRecursively(Zext, AM, Depth + 1);
       return false;
     }
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/82881