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

[FLZ via Phabricator via llvm-commits]

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() &&
----------------
FLZ101 wrote:
> 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.

> the combining won't produce a scalable vector

should be

> the combining will produce a scalable vector


================
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();
----------------
FLZ101 wrote:
> 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.

> the combining won't produce a scalable vector

should be

> the combining will produce a scalable vector


================
Comment at: llvm/test/CodeGen/AArch64/dag-combine-concat-vectors.ll:1
+; RUN: llc -mtriple=aarch64-none-eabi -mattr=+sve < %s
+; check whether commands above could exit normally rather than get stuck in the infinite loop
----------------
RKSimon wrote:
> Add a FileCheck and run update_llc_test_checks to generate codegen to test
Is there a way to check whether the case could be finished in for example 10s ? I think that would be more appropriate


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

https://reviews.llvm.org/D153316