[Analysis] Bail out for negative offsets in isDereferenceableAndAlignedInLoop

[david-arm]

This patch is effectively NFC because it doesn't actually affect any existing tests. It's not possible to even write a test for it, because by accident negative offsets are caught by a subsequent unsigned add overflow check. However, I think it's better to bail out explicitly for negative offsets so that it's more consistent with the unsigned remainder and add calculations.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: David Sherwood (david-arm)

Changes

This patch is effectively NFC because it doesn't actually affect any existing tests. It's not possible to even write a test for it, because by accident negative offsets are caught by a subsequent unsigned add overflow check. However, I think it's better to bail out explicitly for negative offsets so that it's more consistent with the unsigned remainder and add calculations.

---

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

1 Files Affected:

• (modified) llvm/lib/Analysis/Loads.cpp (+7)

diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 1d54a66705a2a..c19a276464e1e 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -313,6 +313,13 @@ bool llvm::isDereferenceableAndAlignedInLoop(LoadInst *LI, Loop *L,
     const auto *Offset = dyn_cast<SCEVConstant>(StartS->getOperand(0));
     const auto *NewBase = dyn_cast<SCEVUnknown>(StartS->getOperand(1));
     if (StartS->getNumOperands() == 2 && Offset && NewBase) {
+      // The following code below assumes the offset is unsigned, but GEP
+      // offsets are treated as signed so we can end up with a signed value
+      // here too. For example, suppose the initial PHI value is (i8 255),
+      // the offset will be treated as (i8 -1) and sign-extended to (i64 -1).
+      if (Offset->getAPInt().isNegative())
+        return false;
+
       // For the moment, restrict ourselves to the case where the offset is a
       // multiple of the requested alignment and the base is aligned.
       // TODO: generalize if a case found which warrants

[fhahn]

Thanks for splitting this off!

I think https://llvm.godbolt.org/z/boYbjenvq may serve as a test case; it is using a pointer width of 16 (via the data layout and an alloca that's spanning more than half the available address space) and the GEP computing the address for the conditional load must wrap I think, which would make the inbounds GEP poison, so we shouldn't execute this unconditionally.

[david-arm]

Thanks for splitting this off!

I think https://llvm.godbolt.org/z/boYbjenvq may serve as a test case; it is using a pointer width of 16 (via the data layout and an alloca that's spanning more than half the available address space) and the GEP computing the address for the conditional load must wrap I think, which would make the inbounds GEP poison, so we shouldn't execute this unconditionally.

Thanks a lot for this @fhahn! Your test has actually exposed a genuine bug that's now fixed by this patch. We were treating loads with negative offsets from allocas as being dereferenceable.

[fhahn]

LGTM, thanks!