[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:
> > 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
Comments updated


================
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:
> > 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
Comments updated


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

https://reviews.llvm.org/D153316