[PATCH] D125248: [instcombine] Propagate freeze back to def

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin created this revision.
mtrofin added reviewers: hyeongyukim, nikic, lebedev.ri.
Herald added a subscriber: hiraditya.
Herald added a project: All.
mtrofin requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

We noticed a regression in
llvm-test-suite/MicroBenchmarks/ImageProcessing/Interpolation that,
through bisection, appeared to be caused by D124252 <https://reviews.llvm.org/D124252>. Upon investigation,
the root cause appears to be the way D105392 <https://reviews.llvm.org/D105392> propagates upwards freezes.

D124252 <https://reviews.llvm.org/D124252> would freeze the `cmp` result, and loop unswitching would
otherwise happen identically. We'd then do instcombine. At this point,
we'd be presented with a few redundant definitions - see the `%idx1` and
`%idx2` in the added test. They'd have uses in the `then` and `else`
sides of the unswitched loop that, before D124252 <https://reviews.llvm.org/D124252>, would be recognized
as equivalent and optimized out (the test glosses over that and just
uses the `mock_use` function to make sure the values have a use).

D124252 <https://reviews.llvm.org/D124252>'s introduction of the `freeze` kicked in the behavior introduced
by D105392 <https://reviews.llvm.org/D105392>. The problem is that the freeze would be always propagated up
right before the current freeze - which in our case would leave one of
the equivalent `idx` before the freeze and one after. The two would be
now different, and we lose the optimization opportunity. Here's what the
IR looked like post-instcombine, for the newly added test:

  define i1 @func(i32 %0, i32 noundef %1) {
    %dr = lshr i32 %0, 2
    %idx1 = zext i32 %dr to i64
    %dr.fr = freeze i32 %dr
    %add = add i32 %dr.fr, 1
    %cmp = icmp slt i32 %add, %1
    %idx2 = zext i32 %dr.fr to i64
    %v = call i1 @use(i64 %idx1, i64 %idx2)
    %ret = and i1 %v, %cmp
    ret i1 %ret
  }

Note how `%idx1` and `%idx2` are using, one the pre-freeze def of `%dr`
and one the post-freeze (i.e. `%dr.fr`).

This patch moves the freeze right after definition.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125248

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
@@ -594,5 +594,26 @@
   ret void
 }
 
-
-
+declare i1 @mock_use(i64, i64)
+define i1 @propagate_freeze(i32 %0, i32 noundef %1) {
+; CHECK-LABEL: @propagate_freeze(
+; CHECK-NEXT:    [[DOTFR:%.*]] = freeze i32 [[TMP0:%.*]]
+; CHECK-NEXT:    [[DR:%.*]] = lshr i32 [[DOTFR]], 2
+; CHECK-NEXT:    [[IDX1:%.*]] = zext i32 [[DR]] to i64
+; CHECK-NEXT:    [[ADD:%.*]] = add nuw nsw i32 [[DR]], 1
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[ADD]], [[TMP1:%.*]]
+; CHECK-NEXT:    [[IDX2:%.*]] = zext i32 [[DR]] to i64
+; CHECK-NEXT:    [[V:%.*]] = call i1 @mock_use(i64 [[IDX1]], i64 [[IDX2]])
+; CHECK-NEXT:    [[RET:%.*]] = and i1 [[V]], [[CMP]]
+; CHECK-NEXT:    ret i1 [[RET]]
+;
+  %dr = lshr i32 %0, 2
+  %idx1 = zext i32 %dr to i64
+  %add = add i32 %dr, 1
+  %cmp = icmp slt i32 %add, %1
+  %cmp.fr = freeze i1 %cmp
+  %idx2 = zext i32 %dr to i64
+  %v = call i1 @mock_use(i64 %idx1, i64 %idx2)
+  %ret = and i1 %v, %cmp.fr
+  ret i1 %ret
+}
Index: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3801,9 +3801,17 @@
 
   auto *FrozenMaybePoisonOperand = new FreezeInst(
       MaybePoisonOperand->get(), MaybePoisonOperand->get()->getName() + ".fr");
-
+  auto *IP = dyn_cast<Instruction>(MaybePoisonOperand->get());
   replaceUse(*MaybePoisonOperand, FrozenMaybePoisonOperand);
-  FrozenMaybePoisonOperand->insertBefore(OrigOpInst);
+  if (IP) {
+    if (isa<PHINode>(IP))
+      IP->getParent()->getInstList().insert(
+          IP->getParent()->getFirstInsertionPt(), FrozenMaybePoisonOperand);
+    else
+      FrozenMaybePoisonOperand->insertAfter(IP);
+  } else {
+    FrozenMaybePoisonOperand->insertBefore(OrigOpInst);
+  }
   return OrigOp;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D125248.428134.patch
Type: text/x-patch
Size: 2072 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220509/ac65d2b5/attachment.bin>