[PATCH] D141256: [DAGCombine]Don't check for Undef/Poison if the node is deleted

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14356
+    return SDValue();
+  }
+  else{
----------------
RKSimon wrote:
> RKSimon wrote:
> > RKSimon wrote:
> > > (style) early out instead of a if-else chain
> > > ```
> > > if(N0.getOpcode() == ISD::DELETED_NODE)
> > >   return SDValue()
> > > // NOTE: this strips poison generating flags.
> > > SDValue R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);
> > > ```
> > Can you confirm that the DELETED_NODE is appearing due to the UpdateNodeOperands call above (sorry you didn't generate the diff with context so I can't point to it)
> > 
> > I really need to step through the debugger myself, but can't right now :( 
> OK - in which case this can be put straight after the UpdateNodeOperands:
> ```
>   for (SDValue MaybePoisonOperand : MaybePoisonOperands) {
>     // Don't replace every single UNDEF everywhere with frozen UNDEF, though.
>     if (MaybePoisonOperand.getOpcode() == ISD::UNDEF)
>       continue;
>     // First, freeze each offending operand.
>     SDValue FrozenMaybePoisonOperand = DAG.getFreeze(MaybePoisonOperand);
>     // Then, change all other uses of unfrozen operand to use frozen operand.
>     DAG.ReplaceAllUsesOfValueWith(MaybePoisonOperand, FrozenMaybePoisonOperand);
>     if (FrozenMaybePoisonOperand.getOpcode() == ISD::FREEZE &&
>         FrozenMaybePoisonOperand.getOperand(0) == FrozenMaybePoisonOperand) {
>       // But, that also updated the use in the freeze we just created, thus
>       // creating a cycle in a DAG. Let's undo that by mutating the freeze.
>       DAG.UpdateNodeOperands(FrozenMaybePoisonOperand.getNode(),
>                              MaybePoisonOperand);
>     }
>   }
> 
>   // Early-out if the node has already been updated in place.
>   if (N0.getOpcode() == ISD::DELETED_NODE)
>     return SDValue();
> 
>   // Finally, recreate the node, it's operands were updated to use
>   // frozen operands, so we just need to use it's "original" operands.
>   SmallVector<SDValue> Ops(N0->op_begin(), N0->op_end());
>   // Special-handle ISD::UNDEF, each single one of them can be it's own thing.
>   for (SDValue &Op : Ops) {
>     if (Op.getOpcode() == ISD::UNDEF)
>       Op = DAG.getFreeze(Op);
>   }
>   // NOTE: this strips poison generating flags.
>   SDValue R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);
>   assert(DAG.isGuaranteedNotToBeUndefOrPoison(R, /*PoisonOnly*/ false) &&
>          "Can't create node that may be undef/poison!");
>   return R;
> ```
@RKSimon if you are sure about just bailing in case of in-place update,
please feel free to land that version of this patch.


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

https://reviews.llvm.org/D141256