[llvm] r353416 - [DAG] Cleanup unused nodes on failed store-to-load forward combine.

[Nirav Davé via llvm-commits]

A set would be better, but order is nominally important. Actually, based on
my work since, we actually need a much more involved general fix which I've
mostly worked out.
If you haven't already I'm going to revert this now.

Thanks,

-Nirav

On Fri, Feb 8, 2019 at 8:21 AM Mikael Holmén <mikael.holmen at ericsson.com>
wrote:

> Hi Nirav,
>
> On 2/7/19 4:38 PM, Nirav Dave via llvm-commits wrote:
> > Author: niravd
> > Date: Thu Feb  7 07:38:14 2019
> > New Revision: 353416
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=353416&view=rev
> > Log:
> > [DAG] Cleanup unused nodes on failed store-to-load forward combine.
> >
> > Modified:
> >      llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >
> > Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=353416&r1=353415&r2=353416&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Thu Feb  7
> 07:38:14 2019
> > @@ -13123,30 +13123,42 @@ SDValue DAGCombiner::ForwardStoreValueTo
> >     if (LD->getBasePtr().isUndef() || Offset != 0)
> >       return SDValue();
> >     // Model necessary truncations / extenstions.
> > -  SDValue Val;
> > +  SmallVector<SDNode *, 4> Vals; // Temporaries which may need to be
> deleted.
> > +  SDValue Val, RV;
> >     // Truncate Value To Stored Memory Size.
> >     do {
> >       if (!getTruncatedStoreValue(ST, Val))
> >         continue;
> > +    if (Vals.empty() || Vals.back() != Val.getNode())
> > +      Vals.push_back(Val.getNode());
> >       if (!isTypeLegal(LDMemType))
> >         continue;
> >       if (STMemType != LDMemType) {
> >         // TODO: Support vectors? This requires
> extract_subvector/bitcast.
> >         if (!STMemType.isVector() && !LDMemType.isVector() &&
> > -          STMemType.isInteger() && LDMemType.isInteger())
> > +          STMemType.isInteger() && LDMemType.isInteger()) {
> > +        Vals.push_back(Val.getNode());
>
> All other places where we add Val.getNode() to Vals we guard that with
>
>   if (Vals.empty() || Vals.back() != Val.getNode())
>
> any reason we shouldn't guard the push_back above too?
>
> I've triggered a case in our downstream clone where the same node is
> added twice to Vals (which causes a failed assertion due to us trying to
> remove the same node twice). Unfortunately my test case doesn't trigger
> the crash on trunk due to some local patches we have.
>
> Alternatively, should Vals be a set rather than a vector?
>
> Regards,
> Mikael
>
> >           Val = DAG.getNode(ISD::TRUNCATE, SDLoc(LD), LDMemType, Val);
> > -      else
> > +      } else
> >           continue;
> >       }
> > -    if (!extendLoadedValueToExtension(LD, Val))
> > -      continue;
> > -    return ReplaceLd(LD, Val, Chain);
> > +    if (Vals.empty() || Vals.back() != Val.getNode())
> > +      Vals.push_back(Val.getNode());
> > +    if (extendLoadedValueToExtension(LD, Val))
> > +      RV = ReplaceLd(LD, Val, Chain);
> > +    else if (Vals.empty() || Vals.back() != Val.getNode())
> > +      Vals.push_back(Val.getNode());
> >     } while (false);
> >
> >     // On failure, cleanup dead nodes we may have created.
> > -  if (Val->use_empty())
> > -    deleteAndRecombine(Val.getNode());
> > -  return SDValue();
> > +  if (Vals.empty() || Vals.back() != Val.getNode())
> > +    Vals.push_back(Val.getNode());
> > +  while (!Vals.empty()) {
> > +    SDNode *Val = Vals.pop_back_val();
> > +    if (Val->use_empty())
> > +      recursivelyDeleteUnusedNodes(Val);
> > +  }
> > +  return RV;
> >   }
> >
> >   SDValue DAGCombiner::visitLOAD(SDNode *N) {
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190208/ba8f3b5c/attachment.html>