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

[Mikael Holmén via llvm-commits]

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
>