[LoopIdiom] PR32811 check for safety while expanding

[hiraditya]

Summary

Loop Idiom recognition was generating memset in a case that would result generating a division operation to an unsafe location.

[davide]

test/Transforms/LoopIdiom/unsafe.ll

2–8

I'm under the impression this test can be greatly reduced.

[efriedma]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

823

Do we also need to check whether it's safe to expand NumBytesS?

[hiraditya]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

823

I don't think so (IIUC), because the NumBytes is based on the trip count of the loop*storesize, both of them should be safe to expand if the loop has a valid SCEV.

[efriedma]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

823

The store size is a constant, so that's obviously fine, but I don't see how the loop's trip count is obviously safe to expand; . Consider, e.g.:

#include <stdio.h>
void f(unsigned a, unsigned b, char* x) {
  for (int j = 0; j < a; j++)
    if (b != 0)
      for (long long i = 0; i < a / b; ++i)
        x[i] = 0;
}

[hiraditya]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

823

Thanks for the test case. You are right, this test does hoist udiv even with this patch. I'll update the patch and add this test case.

[hiraditya]

Addressed Eli's comments.

[efriedma]

LGTM, with some minor comments.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

825

We should rearrange the checks so LIR doesn't leave around the dead expansion of BasePtr... but it's not a big deal, and it would make this patch a mess, so please fix that as a followup.

test/Transforms/LoopIdiom/unsafe.ll

25

Using "undef" here is kind of confusing; please use 0 instead.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

   Type *DestInt8PtrTy = Builder.getInt8PtrTy(DestAS);
   Type *IntPtr = Builder.getIntPtrTy(*DL, DestAS);
 
   const SCEV *Start = Ev->getStart();
   // Handle negative strided loops.
   if (NegStride)
     Start = getStartForNegStride(Start, BECount, IntPtr, StoreSize, SE);
 
+  // TODO: ideally we should still be able to generate memset if SCEV expander
+  // is taught to generate the dependencies at the latest point.
+  if (!isSafeToExpand(Start, *SE))
+    return false;
+
   // Okay, we have a strided store "p[i]" of a splattable value.  We can turn
   // this into a memset in the loop preheader now if we want.  However, this
   // would be unsafe to do if there is anything else in the loop that may read
   // or write to the aliased location.  Check for any overlap by generating the
   // base pointer and checking the region.
   Value *BasePtr =
       Expander.expandCodeFor(Start, DestInt8PtrTy, Preheader->getTerminator());
   if (mayLoopAccessLocation(BasePtr, MRI_ModRef, CurLoop, BECount, StoreSize,

   const SCEV *NumBytesS =
       SE->getAddExpr(BECount, SE->getOne(IntPtr), SCEV::FlagNUW);
   if (StoreSize != 1) {
     NumBytesS = SE->getMulExpr(NumBytesS, SE->getConstant(IntPtr, StoreSize),
                                SCEV::FlagNUW);
   }
 
   Value *NumBytes =
       Expander.expandCodeFor(NumBytesS, IntPtr, Preheader->getTerminator());
 
   CallInst *NewCall;
   if (SplatValue) {
     NewCall =
         Builder.CreateMemSet(BasePtr, SplatValue, NumBytes, StoreAlignment);
   } else {
     // Everything is emitted in default address space
     Type *Int8PtrTy = DestInt8PtrTy;
 
     Module *M = TheStore->getModule();

test/Transforms/LoopIdiom/unsafe.ll

+; RUN: opt -S < %s -loop-idiom | FileCheck %s
+; CHECK-NOT: memset
+; check that memset is not generated (for assignment in for.body5) because that will result
+; in udiv hoisted out of the loop by the SCEV Expander
+; TODO: ideally we should be able to generate memset
+; if SCEV expander is taught to generate the dependencies
+; at the right point.
+
+@a = global i32 0, align 4
+@b = global i32 0, align 4
+@c = external local_unnamed_addr global [1 x i8], align 1
+
+define void @main() local_unnamed_addr {
+entry:
+  %d0 = load i32, i32* @a, align 4
+  %d1 = load i32, i32* @b, align 4
+  br label %for.cond1thread-pre-split
+
+for.cond1thread-pre-split:                        ; preds = %for.body5, %entry
+  %div = udiv i32 %d0, %d1
+  br label %for.body5
+
+for.body5:                                        ; preds = %for.body5, %for.cond1thread-pre-split
+  %indvars.iv = phi i64 [ undef, %for.cond1thread-pre-split ], [ %indvars.iv.next, %for.body5 ]
+  %divx = sext i32 %div to i64
+  %0 = add nsw i64 %divx, %indvars.iv
+  %arrayidx = getelementptr inbounds [1 x i8], [1 x i8]* @c, i64 0, i64 %0
+  store i8 0, i8* %arrayidx, align 1
+  %indvars.iv.next = add nsw i64 %indvars.iv, 1
+  %1 = trunc i64 %indvars.iv.next to i32
+  %tobool4 = icmp eq i32 %1, 0
+  br i1 %tobool4, label %for.cond1thread-pre-split, label %for.body5
+}