<div dir="ltr">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. <div>If you haven't already I'm going to revert this now. <br></div><div><br></div><div>Thanks,</div><div><br></div><div>-Nirav</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 8, 2019 at 8:21 AM Mikael Holmén <<a href="mailto:mikael.holmen@ericsson.com">mikael.holmen@ericsson.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Nirav,<br>
<br>
On 2/7/19 4:38 PM, Nirav Dave via llvm-commits wrote:<br>
> Author: niravd<br>
> Date: Thu Feb  7 07:38:14 2019<br>
> New Revision: 353416<br>
> <br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=353416&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=353416&view=rev</a><br>
> Log:<br>
> [DAG] Cleanup unused nodes on failed store-to-load forward combine.<br>
> <br>
> Modified:<br>
>      llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> <br>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=353416&r1=353415&r2=353416&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=353416&r1=353415&r2=353416&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Thu Feb  7 07:38:14 2019<br>
> @@ -13123,30 +13123,42 @@ SDValue DAGCombiner::ForwardStoreValueTo<br>
>     if (LD->getBasePtr().isUndef() || Offset != 0)<br>
>       return SDValue();<br>
>     // Model necessary truncations / extenstions.<br>
> -  SDValue Val;<br>
> +  SmallVector<SDNode *, 4> Vals; // Temporaries which may need to be deleted.<br>
> +  SDValue Val, RV;<br>
>     // Truncate Value To Stored Memory Size.<br>
>     do {<br>
>       if (!getTruncatedStoreValue(ST, Val))<br>
>         continue;<br>
> +    if (Vals.empty() || Vals.back() != Val.getNode())<br>
> +      Vals.push_back(Val.getNode());<br>
>       if (!isTypeLegal(LDMemType))<br>
>         continue;<br>
>       if (STMemType != LDMemType) {<br>
>         // TODO: Support vectors? This requires extract_subvector/bitcast.<br>
>         if (!STMemType.isVector() && !LDMemType.isVector() &&<br>
> -          STMemType.isInteger() && LDMemType.isInteger())<br>
> +          STMemType.isInteger() && LDMemType.isInteger()) {<br>
> +        Vals.push_back(Val.getNode());<br>
<br>
All other places where we add Val.getNode() to Vals we guard that with<br>
<br>
  if (Vals.empty() || Vals.back() != Val.getNode())<br>
<br>
any reason we shouldn't guard the push_back above too?<br>
<br>
I've triggered a case in our downstream clone where the same node is <br>
added twice to Vals (which causes a failed assertion due to us trying to <br>
remove the same node twice). Unfortunately my test case doesn't trigger <br>
the crash on trunk due to some local patches we have.<br>
<br>
Alternatively, should Vals be a set rather than a vector?<br>
<br>
Regards,<br>
Mikael<br>
<br>
>           Val = DAG.getNode(ISD::TRUNCATE, SDLoc(LD), LDMemType, Val);<br>
> -      else<br>
> +      } else<br>
>           continue;<br>
>       }<br>
> -    if (!extendLoadedValueToExtension(LD, Val))<br>
> -      continue;<br>
> -    return ReplaceLd(LD, Val, Chain);<br>
> +    if (Vals.empty() || Vals.back() != Val.getNode())<br>
> +      Vals.push_back(Val.getNode());<br>
> +    if (extendLoadedValueToExtension(LD, Val))<br>
> +      RV = ReplaceLd(LD, Val, Chain);<br>
> +    else if (Vals.empty() || Vals.back() != Val.getNode())<br>
> +      Vals.push_back(Val.getNode());<br>
>     } while (false);<br>
>   <br>
>     // On failure, cleanup dead nodes we may have created.<br>
> -  if (Val->use_empty())<br>
> -    deleteAndRecombine(Val.getNode());<br>
> -  return SDValue();<br>
> +  if (Vals.empty() || Vals.back() != Val.getNode())<br>
> +    Vals.push_back(Val.getNode());<br>
> +  while (!Vals.empty()) {<br>
> +    SDNode *Val = Vals.pop_back_val();<br>
> +    if (Val->use_empty())<br>
> +      recursivelyDeleteUnusedNodes(Val);<br>
> +  }<br>
> +  return RV;<br>
>   }<br>
>   <br>
>   SDValue DAGCombiner::visitLOAD(SDNode *N) {<br>
> <br>
> <br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
> <br>
</blockquote></div>