[PATCH] D105392: [InstCombine] Add optimization to prevent poison from being propagated.

[Hyeongyu Kim via Phabricator via llvm-commits]

hyeongyukim updated this revision to Diff 356443.
hyeongyukim added a comment.

Add assertion about PHINode


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105392

Files:
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/Transforms/InstCombine/freeze.ll


Index: llvm/test/Transforms/InstCombine/freeze.ll
===================================================================
--- llvm/test/Transforms/InstCombine/freeze.ll
+++ llvm/test/Transforms/InstCombine/freeze.ll
@@ -105,6 +105,7 @@
 }
 
 define i1 @early_freeze_test2(i32* %ptr) {
+;
 ; CHECK-LABEL: @early_freeze_test2(
 ; CHECK-NEXT:    [[V1:%.*]] = load i32, i32* [[PTR:%.*]], align 4
 ; CHECK-NEXT:    [[V1_FR:%.*]] = freeze i32 [[V1]]
Index: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3520,7 +3520,7 @@
   // guaranteed-non-poison operands then push the freeze through to the one
   // operand that is not guaranteed non poison.  The actual transform is as
   // follows.
-  //   Op1 = ... ;                      ; Op1 can be posion
+  //   Op1 = ...                        ; Op1 can be posion
   //   Op0 = Inst(Op1, NonPoisonOps...) ; Op0 has only one use and only have
   //                                    ; single guaranteed-non-poison operands
   //   ... = Freeze(Op0)
@@ -3528,31 +3528,50 @@
   //   Op1 = ...
   //   Op1.fr = Freeze(Op1)
   //   ... = Inst(Op1.fr, NonPoisonOps...)
-  auto* OrigOp = OrigFI.getOperand(0);
-  if (auto *OrigOpInst = dyn_cast<Instruction>(OrigOp)) {
-    if (OrigOpInst->hasOneUse() && !canCreateUndefOrPoison(dyn_cast<Operator>(OrigOp))) {
-      Optional<Use *> MaybePoisonOperand;
-      for (Use &U : OrigOpInst->operands()) {
-        if (isGuaranteedNotToBeUndefOrPoison(U.get()))
-          continue;
-        if (!MaybePoisonOperand)
-          MaybePoisonOperand = &U;
-        else if(MaybePoisonOperand)
-          return nullptr;
-      }
+  auto *OrigOp = OrigFI.getOperand(0);
+  auto *OrigOpInst = dyn_cast<Instruction>(OrigOp);
 
-      if (!MaybePoisonOperand.hasValue())
-        return nullptr;
+  // We already folded "freeze (phi const, x)" to "phi const, (freeze x)"
+  assert(!isa<PHINode>(OrigOp));
 
-      auto *MaybePoisonUse = MaybePoisonOperand.getValue();
-      auto *FrozenMaybePoisonOperand = new FreezeInst(MaybePoisonUse->get(), MaybePoisonUse->getUser()->getName() + ".fr");
+  // If Op0 is used in places other then freeze, wrong transformation will occur as below.
+  //   Op1 = ...
+  //   Op0 = Inst(Op1, NonPoisonOps...)
+  //   Use(Op0)             ; Op0 is not frozen.
+  //   ... = Freeze(Op0)
+  // =>
+  //   Op1 = ...
+  //   Op1.fr = Freeze(Op1)
+  //   Op0.fr = Inst(Op1.fr, NonPoisonOps...)
+  //   Use(Op0.fr)          ; Op0.fr is already frozen. Wrong Transformation.
+  if (!OrigOpInst || !OrigOpInst->hasOneUse() ||
+      canCreateUndefOrPoison(dyn_cast<Operator>(OrigOp)))
+    return nullptr;
 
-      replaceUse(*MaybePoisonUse, FrozenMaybePoisonOperand);
-      FrozenMaybePoisonOperand->insertBefore(OrigOpInst);
-      return OrigOp;
-    }
+  // If operand is guaranteed not to be poison, there is no need to add freeze
+  // to the operand. So we first find the operand that is not guranteed to be
+  // poison.
+  Use *MaybePoisonOperand = nullptr;
+  for (Use &U : OrigOpInst->operands()) {
+    if (isGuaranteedNotToBeUndefOrPoison(U.get()))
+      continue;
+    if (!MaybePoisonOperand)
+      MaybePoisonOperand = &U;
+    else
+      return nullptr;
   }
-  return nullptr;
+
+  // If all operands are guaranteed to be non-poison, we can drop freeze.
+  if (!MaybePoisonOperand)
+    return OrigOp;
+
+  auto *FrozenMaybePoisonOperand =
+      new FreezeInst(MaybePoisonOperand->get(),
+                     MaybePoisonOperand->get()->getName() + ".fr");
+
+  replaceUse(*MaybePoisonOperand, FrozenMaybePoisonOperand);
+  FrozenMaybePoisonOperand->insertBefore(OrigOpInst);
+  return OrigOp;
 }
 
 Instruction *InstCombinerImpl::visitFreeze(FreezeInst &I) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105392.356443.patch
Type: text/x-patch
Size: 3884 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210705/55fff63b/attachment.bin>