[Analysis] Add new function isDereferenceableReadOnlyLoop

[david-arm]

I created this patch due to a reviewer request on PR #88385 to split off the analysis changes, however without the other code in that PR I can only test the new function with unit tests.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: David Sherwood (david-arm)

Changes

I created this patch due to a reviewer request on PR #88385 to split off the analysis changes, however without the other code in that PR I can only test the new function with unit tests.

---

Full diff: https://github.com/llvm/llvm-project/pull/97292.diff

3 Files Affected:

• (modified) llvm/include/llvm/Analysis/Loads.h (+5)
• (modified) llvm/lib/Analysis/Loads.cpp (+16)
• (modified) llvm/unittests/Analysis/LoadsTest.cpp (+88)

diff --git a/llvm/include/llvm/Analysis/Loads.h b/llvm/include/llvm/Analysis/Loads.h
index a8d954b9872d9..370414b26affd 100644
--- a/llvm/include/llvm/Analysis/Loads.h
+++ b/llvm/include/llvm/Analysis/Loads.h
@@ -86,6 +86,11 @@ bool isDereferenceableAndAlignedInLoop(LoadInst *LI, Loop *L,
                                        ScalarEvolution &SE, DominatorTree &DT,
                                        AssumptionCache *AC = nullptr);
 
+/// Return true if the loop \p L cannot fault on any iteration and only
+/// contains read-only memory accesses.
+bool isDereferenceableReadOnlyLoop(Loop *L, ScalarEvolution *SE,
+                                   DominatorTree *DT, AssumptionCache *AC);
+
 /// Return true if we know that executing a load from this value cannot trap.
 ///
 /// If DT and ScanFrom are specified this method performs context-sensitive
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 2b8197066e8e9..cc36509a7a817 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -769,3 +769,19 @@ bool llvm::canReplacePointersIfEqual(const Value *From, const Value *To,
 
   return isPointerAlwaysReplaceable(From, To, DL);
 }
+
+bool llvm::isDereferenceableReadOnlyLoop(Loop *L, ScalarEvolution *SE,
+                                         DominatorTree *DT,
+                                         AssumptionCache *AC) {
+  for (BasicBlock *BB : L->blocks()) {
+    for (Instruction &I : *BB) {
+      LoadInst *LI = dyn_cast<LoadInst>(&I);
+      if (LI) {
+        if (!isDereferenceableAndAlignedInLoop(LI, L, *SE, *DT, AC))
+          return false;
+      } else if (I.mayReadFromMemory() || I.mayWriteToMemory() || I.mayThrow())
+        return false;
+    }
+  }
+  return true;
+}
diff --git a/llvm/unittests/Analysis/LoadsTest.cpp b/llvm/unittests/Analysis/LoadsTest.cpp
index 5da3feaf762f3..4c01d481c6f69 100644
--- a/llvm/unittests/Analysis/LoadsTest.cpp
+++ b/llvm/unittests/Analysis/LoadsTest.cpp
@@ -7,8 +7,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Analysis/Loads.h"
+#include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/LoopInfo.h"
+#include "llvm/Analysis/ScalarEvolution.h"
+#include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/Dominators.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
@@ -114,3 +119,86 @@ define void @f(i32* %p1, i32* %p2, i64 %i) {
   EXPECT_TRUE(canReplacePointersInUseIfEqual(PtrToIntUse, P2, DL));
   EXPECT_TRUE(canReplacePointersInUseIfEqual(IcmpUse, P2, DL));
 }
+
+TEST(LoadsTest, IsDerefReadOnlyLoop) {
+  LLVMContext C;
+  std::unique_ptr<Module> M = parseIR(C,
+                                      R"IR(
+define i64 @f1() {
+entry:
+  %p1 = alloca [1024 x i8]
+  %p2 = alloca [1024 x i8]
+  br label %loop
+
+loop:
+  %index = phi i64 [ %index.next, %loop.inc ], [ 3, %entry ]
+  %arrayidx = getelementptr inbounds i8, ptr %p1, i64 %index
+  %ld1 = load i8, ptr %arrayidx, align 1
+  %arrayidx1 = getelementptr inbounds i8, ptr %p2, i64 %index
+  %ld2 = load i8, ptr %arrayidx1, align 1
+  %cmp3 = icmp eq i8 %ld1, %ld2
+  br i1 %cmp3, label %loop.inc, label %loop.end
+
+loop.inc:
+  %index.next = add i64 %index, 1
+  %exitcond = icmp ne i64 %index.next, 67
+  br i1 %exitcond, label %loop, label %loop.end
+
+loop.end:
+  %retval = phi i64 [ %index, %loop ], [ 67, %loop.inc ]
+  ret i64 %retval
+}
+
+define i64 @f2(ptr %p1) {
+entry:
+  %p2 = alloca [1024 x i8]
+  br label %loop
+
+loop:
+  %index = phi i64 [ %index.next, %loop.inc ], [ 3, %entry ]
+  %arrayidx = getelementptr inbounds i8, ptr %p1, i64 %index
+  %ld1 = load i8, ptr %arrayidx, align 1
+  %arrayidx1 = getelementptr inbounds i8, ptr %p2, i64 %index
+  %ld2 = load i8, ptr %arrayidx1, align 1
+  %cmp3 = icmp eq i8 %ld1, %ld2
+  br i1 %cmp3, label %loop.inc, label %loop.end
+
+loop.inc:
+  %index.next = add i64 %index, 1
+  %exitcond = icmp ne i64 %index.next, 67
+  br i1 %exitcond, label %loop, label %loop.end
+
+loop.end:
+  %retval = phi i64 [ %index, %loop ], [ 67, %loop.inc ]
+  ret i64 %retval
+}
+)IR");
+  auto *GV1 = M->getNamedValue("f1");
+  auto *GV2 = M->getNamedValue("f2");
+  ASSERT_TRUE(GV1 && GV2);
+  auto *F1 = dyn_cast<Function>(GV1);
+  auto *F2 = dyn_cast<Function>(GV2);
+  ASSERT_TRUE(F1 && F2);
+  Function *FNs[2] = { F1, F2 };
+
+  TargetLibraryInfoImpl TLII;
+  TargetLibraryInfo TLI(TLII);
+  for (unsigned I = 0; I < 2; I++) {
+    Function *F = FNs[I];
+    AssumptionCache AC(*F);
+    DominatorTree DT(*F);
+    LoopInfo LI(DT);
+    ScalarEvolution SE(*F, TLI, AC, DT, LI);
+
+    Function::iterator FI = F->begin();
+    // First basic block is entry - skip it.
+    BasicBlock *Header = &*(++FI);
+    assert(Header->getName() == "loop");
+    Loop *L = LI.getLoopFor(Header);
+
+    if (I == 0)
+      ASSERT_TRUE(isDereferenceableReadOnlyLoop(L, &SE, &DT, &AC));
+    else
+      ASSERT_FALSE(isDereferenceableReadOnlyLoop(L, &SE, &DT, &AC));
+  }
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[david-arm]

• Rebase

[huntergr-arm]

I think it might be cleaner if you added a lambda function to perform the lookup and analysis, then return the result of isDereferenceableReadOnlyLoop and assert on the return value.

That way you wouldn't need the loop and the if/else based on I.

[david-arm]

Done!

[huntergr-arm]

nit: combine with the above line?

Suggested change

	if (LI) {
	if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {

[david-arm]

Done!

[huntergr-arm]

LGTM.

I did look into isSafeToSpeculativelyExecute to see whether that was an appropriate condition for the instructions in the loop, but I think it goes a bit further than the intended meaning of this function so we're fine without using it.