[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:
> 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
> So if we skip the combining here when any of the following conditions is met

should be

> So if we skip the combining here when the following conditions are met


================
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:
> 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
> So if we skip the combining here when any of the following conditions is met

should be

> So if we skip the combining here when the following conditions are met


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

https://reviews.llvm.org/D153316