[PATCH] D116754: Bugfix for application of trivial loop optimization in LoopIdiomRecognize(Github Issue #50308)

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:628-636
   // We can only promote stores in this block if they are unconditionally
-  // executed in the loop.  For a block to be unconditionally executed, it has
-  // to dominate all the exit blocks of the loop.  Verify this now.
-  for (BasicBlock *ExitBlock : ExitBlocks)
-    if (!DT->dominates(BB, ExitBlock))
-      return false;
+  // executed in the loop. For a block to be unconditionally executed, it has
+  // to dominate all the exit blocks of the loop. Verify this now.
+  // On countable unrotated loops, checking the exit blocks fails.
+  // Hence, this is only run on rotated loops.
+  if (isFromRotatedLoop)
+    for (BasicBlock *ExitBlock : ExitBlocks)
----------------
razetime wrote:
> lebedev.ri wrote:
> > I do not understand why we can just not do this check for unrotated loops?
> The check unconditionally failed on the unrotated test case I was using from https://llvm.godbolt.org/z/YrGqGhnE9.
> 
> The place where it failed was when BB =`loop.latch` and ExitBlock = `exit`. pre-rotating the loop would work, but it seemed unintuitive and costlier than skipping the check. 
It might not be a good reason to remove a check simply if it fails on a false negative condition. Since the condition might do many other true negative checks.


================
Comment at: llvm/test/Transforms/LoopIdiom/memset-unrotated-loop.ll:7-11
+; CHECK-NEXT:    [[START2:%.*]] = ptrtoint i8* [[START:%.*]] to i64
+; CHECK-NEXT:    [[END1:%.*]] = ptrtoint i8* [[END:%.*]] to i64
+; CHECK-NEXT:    [[TMP0:%.*]] = add i64 [[END1]], 1
+; CHECK-NEXT:    [[TMP1:%.*]] = sub i64 [[TMP0]], [[START2]]
+; CHECK-NEXT:    call void @llvm.memset.p0i8.i64(i8* align 1 [[START]], i8 1, i64 [[TMP1]], i1 false)
----------------
This transformation looks not good. Given %start is not no alias and %start == %end, then the original behavior would be no-op. However, it would set value `1` at %start in the transformed code. See https://llvm.godbolt.org/z/a9czn4z4b, which contains the original semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116754