[DAG] visitFREEZE - nodes that allow multiple frozen operands should directly freeze all operands

Avoid the ReplaceAllUsesOfValueWith approach that has caused so many problems in previous attempts at this (#145939).

Minor step towards #149798 - eventually we're just going to use this path for all node types, but there's a large number of regressions that need addressing first (see #152107).

This exposed a couple of things that need to be addressed here:
- we need to revisit frozen nodes once we've merged all frozen/unfrozen uses of a node.
- RISCVISD::READ_VLENB is never undef/poison

Many of the current regressions are due to us not doing more to avoid freeze(load) nodes - if the load is legal, its no longer going to split. I'm not certain exactly when we can split nodes - #160884 tried to catch up to ValueTracking but that might still be wrong.

Author: RKSimon

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

@@ -16918,6 +16918,8 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
     // creating a cycle in a DAG. Let's undo that by mutating the freeze.
     assert(N->getOperand(0) == FrozenN0 && "Expected cycle in DAG");
     DAG.UpdateNodeOperands(N, N0);
+    // Revisit the node.
+    AddToWorklist(N);
     return FrozenN0;
   }
 
@@ -16972,72 +16974,81 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
     }
   }
 
-  SmallSet<SDValue, 8> MaybePoisonOperands;
-  SmallVector<unsigned, 8> MaybePoisonOperandNumbers;
-  for (auto [OpNo, Op] : enumerate(N0->ops())) {
-    if (DAG.isGuaranteedNotToBeUndefOrPoison(Op, /*PoisonOnly=*/false))
-      continue;
-    bool HadMaybePoisonOperands = !MaybePoisonOperands.empty();
-    bool IsNewMaybePoisonOperand = MaybePoisonOperands.insert(Op).second;
-    if (IsNewMaybePoisonOperand)
-      MaybePoisonOperandNumbers.push_back(OpNo);
-    if (!HadMaybePoisonOperands)
-      continue;
-    if (IsNewMaybePoisonOperand && !AllowMultipleMaybePoisonOperands) {
-      // Multiple maybe-poison ops when not allowed - bail out.
-      return SDValue();
+  SmallVector<SDValue> Ops;
+  if (AllowMultipleMaybePoisonOperands) {
+    // Collect and freeze all operands.
+    Ops = SmallVector<SDValue>(N0->ops());
+    for (SDValue &Op : Ops)
+      Op = DAG.getFreeze(Op);
+  } else {
+    SmallSet<SDValue, 8> MaybePoisonOperands;
+    SmallVector<unsigned, 8> MaybePoisonOperandNumbers;
+    for (auto [OpNo, Op] : enumerate(N0->ops())) {
+      if (DAG.isGuaranteedNotToBeUndefOrPoison(Op, /*PoisonOnly=*/false))
+        continue;
+      bool HadMaybePoisonOperands = !MaybePoisonOperands.empty();
+      bool IsNewMaybePoisonOperand = MaybePoisonOperands.insert(Op).second;
+      if (IsNewMaybePoisonOperand)
+        MaybePoisonOperandNumbers.push_back(OpNo);
+      if (!HadMaybePoisonOperands)
+        continue;
+      if (IsNewMaybePoisonOperand) {
+        // Multiple maybe-poison ops when not allowed - bail out.
+        return SDValue();
+      }
+    }
+    // NOTE: the whole op may be not guaranteed to not be undef or poison
+    // because it could create undef or poison due to it's poison-generating
+    // flags. So not finding any maybe-poison operands is fine.
+
+    for (unsigned OpNo : MaybePoisonOperandNumbers) {
+      // N0 can mutate during iteration, so make sure to refetch the maybe
+      // poison operands via the operand numbers. The typical scenario is that
+      // we have something like this
+      //   t262: i32 = freeze t181
+      //   t150: i32 = ctlz_zero_undef t262
+      //   t184: i32 = ctlz_zero_undef t181
+      //   t268: i32 = select_cc t181, Constant:i32<0>, t184, t186, setne:ch
+      // When freezing the t181 operand we get t262 back, and then the
+      // ReplaceAllUsesOfValueWith call will not only replace t181 by t262, but
+      // also recursively replace t184 by t150.
+      SDValue MaybePoisonOperand = N->getOperand(0).getOperand(OpNo);
+      // Don't replace every single UNDEF everywhere with frozen UNDEF, though.
+      if (MaybePoisonOperand.isUndef())
+        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);
+      }
+
+      // This node has been merged with another.
+      if (N->getOpcode() == ISD::DELETED_NODE)
+        return SDValue(N, 0);
     }
-  }
-  // NOTE: the whole op may be not guaranteed to not be undef or poison because
-  // it could create undef or poison due to it's poison-generating flags.
-  // So not finding any maybe-poison operands is fine.
-
-  for (unsigned OpNo : MaybePoisonOperandNumbers) {
-    // N0 can mutate during iteration, so make sure to refetch the maybe poison
-    // operands via the operand numbers. The typical scenario is that we have
-    // something like this
-    //   t262: i32 = freeze t181
-    //   t150: i32 = ctlz_zero_undef t262
-    //   t184: i32 = ctlz_zero_undef t181
-    //   t268: i32 = select_cc t181, Constant:i32<0>, t184, t186, setne:ch
-    // When freezing the t181 operand we get t262 back, and then the
-    // ReplaceAllUsesOfValueWith call will not only replace t181 by t262, but
-    // also recursively replace t184 by t150.
-    SDValue MaybePoisonOperand = N->getOperand(0).getOperand(OpNo);
-    // Don't replace every single UNDEF everywhere with frozen UNDEF, though.
-    if (MaybePoisonOperand.isUndef())
-      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);
-    }
-
-    // This node has been merged with another.
-    if (N->getOpcode() == ISD::DELETED_NODE)
-      return SDValue(N, 0);
-  }
 
-  assert(N->getOpcode() != ISD::DELETED_NODE && "Node was deleted!");
+    assert(N->getOpcode() != ISD::DELETED_NODE && "Node was deleted!");
 
-  // The whole node may have been updated, so the value we were holding
-  // may no longer be valid. Re-fetch the operand we're `freeze`ing.
-  N0 = N->getOperand(0);
+    // The whole node may have been updated, so the value we were holding
+    // may no longer be valid. Re-fetch the operand we're `freeze`ing.
+    N0 = N->getOperand(0);
 
-  // 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->ops());
-  // TODO: ISD::UNDEF and ISD::POISON should get separate handling, but best
-  // leave for a future patch.
-  for (SDValue &Op : Ops) {
-    if (Op.isUndef())
-      Op = DAG.getFreeze(Op);
+    // Finally, recreate the node, it's operands were updated to use
+    // frozen operands, so we just need to use it's "original" operands.
+    Ops = SmallVector<SDValue>(N0->ops());
+    // TODO: ISD::UNDEF and ISD::POISON should get separate handling, but best
+    // leave for a future patch.
+    for (SDValue &Op : Ops) {
+      if (Op.isUndef())
+        Op = DAG.getFreeze(Op);
+    }
   }
 
   SDLoc DL(N0);

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

@@ -21918,6 +21918,8 @@ bool RISCVTargetLowering::canCreateUndefOrPoisonForTargetNode(
 
   // TODO: Add more target nodes.
   switch (Op.getOpcode()) {
+  case RISCVISD::READ_VLENB:
+    return false;
   case RISCVISD::SLLW:
   case RISCVISD::SRAW:
   case RISCVISD::SRLW:

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-fold-binop-select.ll

@@ -129,7 +129,7 @@ define i32 @select_sdiv_lhs_opaque_const0_i32(i1 %cond) {
 ; GCN-NEXT:    s_getpc_b64 s[4:5]
 ; GCN-NEXT:    s_add_u32 s4, s4, gv@gotpcrel32@lo+4
 ; GCN-NEXT:    s_addc_u32 s5, s5, gv@gotpcrel32@hi+12
-; GCN-NEXT:    s_load_dword s4, s[4:5], 0x0
+; GCN-NEXT:    s_load_dwordx2 s[4:5], s[4:5], 0x0
 ; GCN-NEXT:    v_and_b32_e32 v0, 1, v0
 ; GCN-NEXT:    v_cmp_eq_u32_e32 vcc, 1, v0
 ; GCN-NEXT:    s_waitcnt lgkmcnt(0)
@@ -211,7 +211,7 @@ define i32 @select_sdiv_lhs_opaque_const1_i32(i1 %cond) {
 ; GCN-NEXT:    s_getpc_b64 s[4:5]
 ; GCN-NEXT:    s_add_u32 s4, s4, gv@gotpcrel32@lo+4
 ; GCN-NEXT:    s_addc_u32 s5, s5, gv@gotpcrel32@hi+12
-; GCN-NEXT:    s_load_dword s4, s[4:5], 0x0
+; GCN-NEXT:    s_load_dwordx2 s[4:5], s[4:5], 0x0
 ; GCN-NEXT:    v_and_b32_e32 v0, 1, v0
 ; GCN-NEXT:    v_cmp_eq_u32_e32 vcc, 1, v0
 ; GCN-NEXT:    s_waitcnt lgkmcnt(0)

llvm/test/CodeGen/AMDGPU/fptoi.i128.ll

@@ -1437,25 +1437,15 @@ define i128 @fptoui_f32_to_i128(float %x) {
 }
 
 define i128 @fptosi_f16_to_i128(half %x) {
-; SDAG-LABEL: fptosi_f16_to_i128:
-; SDAG:       ; %bb.0:
-; SDAG-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; SDAG-NEXT:    v_cvt_f32_f16_e32 v0, v0
-; SDAG-NEXT:    v_cvt_i32_f32_e32 v0, v0
-; SDAG-NEXT:    v_ashrrev_i32_e32 v1, 31, v0
-; SDAG-NEXT:    v_ashrrev_i32_e32 v2, 31, v1
-; SDAG-NEXT:    v_mov_b32_e32 v3, v2
-; SDAG-NEXT:    s_setpc_b64 s[30:31]
-;
-; GISEL-LABEL: fptosi_f16_to_i128:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GISEL-NEXT:    v_cvt_f32_f16_e32 v0, v0
-; GISEL-NEXT:    v_cvt_i32_f32_e32 v0, v0
-; GISEL-NEXT:    v_ashrrev_i32_e32 v1, 31, v0
-; GISEL-NEXT:    v_mov_b32_e32 v2, v1
-; GISEL-NEXT:    v_mov_b32_e32 v3, v1
-; GISEL-NEXT:    s_setpc_b64 s[30:31]
+; GCN-LABEL: fptosi_f16_to_i128:
+; GCN:       ; %bb.0:
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cvt_f32_f16_e32 v0, v0
+; GCN-NEXT:    v_cvt_i32_f32_e32 v0, v0
+; GCN-NEXT:    v_ashrrev_i32_e32 v1, 31, v0
+; GCN-NEXT:    v_mov_b32_e32 v2, v1
+; GCN-NEXT:    v_mov_b32_e32 v3, v1
+; GCN-NEXT:    s_setpc_b64 s[30:31]
   %cvt = fptosi half %x to i128
   ret i128 %cvt
 }