[PATCH] D153316: [AArch64][SelectionDAG] fix infinite loop caused by legalizing & combining CONCAT_VECTORS

[FLZ via Phabricator via llvm-commits]

FLZ101 marked 4 inline comments as done.
FLZ101 added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:23180
+    // make sure targets which does not support SVE are not affected by this
+    // change.
+    if (In.getOpcode() == ISD::CONCAT_VECTORS && In.hasOneUse() &&
----------------
RKSimon wrote:
> The comment doesn't appear to match the code - the code just prevents folding scalable vectors after LegalDAG.
The legalizing happens only when the following conditions are met:

**LegalDAG is true**

`DAGCombiner::Run()`:

```
    // If this combine is running after legalizing the DAG, re-legalize any
    // nodes pulled off the worklist.
    if (LegalDAG) {
      SmallSetVector<SDNode *, 16> UpdatedNodes;
      bool NIsValid = DAG.LegalizeOp(N, UpdatedNodes);

      for (SDNode *LN : UpdatedNodes)
        AddToWorklistWithUsers(LN);

      if (!NIsValid)
        continue;
    }
```

**input of LowerCONCAT_VECTORS is scalable**

`AArch64TargetLowering::LowerCONCAT_VECTORS()`:

```
  assert(Op.getValueType().isScalableVector() &&
         isTypeLegal(Op.getValueType()) &&
         "Expected legal scalable vector type!");
```

So if we skip the combining here when any of the following conditions is met, the infinite loop will not happen:

* LegalDAG is true
* the combining won't produce a scalable vector

I also want targets that does not support SVE are not affected by this change. However I am not sure whether the code does exactly what I want.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:23182
+    if (In.getOpcode() == ISD::CONCAT_VECTORS && In.hasOneUse() &&
+        (!In.getValueType().isScalableVector() || !LegalDAG)) {
       unsigned NumOps = N->getNumOperands() * In.getNumOperands();
----------------
dmgreen wrote:
> Does this need to check for isScalableVector, or can it just check for !LegalDAG? Or does that alter other tests?
The legalizing happens only when the following conditions are met:

**LegalDAG is true**

`DAGCombiner::Run()`:

```
    // If this combine is running after legalizing the DAG, re-legalize any
    // nodes pulled off the worklist.
    if (LegalDAG) {
      SmallSetVector<SDNode *, 16> UpdatedNodes;
      bool NIsValid = DAG.LegalizeOp(N, UpdatedNodes);

      for (SDNode *LN : UpdatedNodes)
        AddToWorklistWithUsers(LN);

      if (!NIsValid)
        continue;
    }
```

**input of LowerCONCAT_VECTORS is scalable**

`AArch64TargetLowering::LowerCONCAT_VECTORS()`:

```
  assert(Op.getValueType().isScalableVector() &&
         isTypeLegal(Op.getValueType()) &&
         "Expected legal scalable vector type!");
```

So if we skip the combining here when any of the following conditions is met, the infinite loop will not happen:

* LegalDAG is true
* the combining won't produce a scalable vector

I also want targets that does not support SVE are not affected by this change. However I am not sure whether the code does exactly what I want.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153316/new/

https://reviews.llvm.org/D153316