[PATCH] D85332: [SCCP] Do not replace deref'able ptr with un-deref'able one.

[Florian Hahn via Phabricator via llvm-commits]

fhahn created this revision.
fhahn added reviewers: nlopes, efriedma, hfinkel, aqjune.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.
fhahn requested review of this revision.

Currently IPSCCP (and others like CVP/GVN) blindly propagate pointer
equalities. In certain cases, that leads to dereferenceable pointers
being replaced, as in the example test case.

I think this is not allowed, as it introduces an access of an
un-dereferenceable pointer. Note that the pointer is inbounds, but one
past the last element, so it is valid, but not dereferenceable.

This patch is mostly to highlight the issue and start a discussion.
Currently it only checks for specifically looking
one-past-the-last-element pointers with array typed bases.

This causes the mis-compile outlined in
https://stackoverflow.com/questions/55754313/is-this-gcc-clang-past-one-pointer-comparison-behavior-conforming-or-non-standar

In the test case, if we replace %p with the GEP for the store, we
subsequently determine that the store and the load cannot alias, because
they are to different underlying objects.

Note that Alive2 seems to think that the replacement is valid:
https://alive2.llvm.org/ce/z/2rorhk


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85332

Files:
  llvm/include/llvm/Analysis/BasicAliasAnalysis.h
  llvm/lib/Transforms/Scalar/SCCP.cpp
  llvm/test/Transforms/SCCP/replace-dereferenceable-ptr-with-undereferenceable.ll


Index: llvm/test/Transforms/SCCP/replace-dereferenceable-ptr-with-undereferenceable.ll
===================================================================
--- llvm/test/Transforms/SCCP/replace-dereferenceable-ptr-with-undereferenceable.ll
+++ llvm/test/Transforms/SCCP/replace-dereferenceable-ptr-with-undereferenceable.ll
@@ -11,7 +11,7 @@
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32* [[P:%.*]], getelementptr inbounds (i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @x, i64 0, i64 0), i64 1)
 ; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    store i32 2, i32* getelementptr inbounds (i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @x, i64 0, i64 0), i64 1), align 4
+; CHECK-NEXT:    store i32 2, i32* [[P]], align 4
 ; CHECK-NEXT:    br label [[IF_END]]
 ; CHECK:       if.end:
 ; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @y, i64 0, i64 0), align 4
Index: llvm/lib/Transforms/Scalar/SCCP.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/SCCP.cpp
+++ llvm/lib/Transforms/Scalar/SCCP.cpp
@@ -26,6 +26,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
 #include "llvm/Analysis/GlobalsModRef.h"
@@ -1264,6 +1265,14 @@
   }
 }
 
+static unsigned getMaxPointerSize(const DataLayout &DL) {
+  unsigned MaxPointerSize = DL.getMaxPointerSizeInBits();
+  if (MaxPointerSize < 64)
+    MaxPointerSize = 64;
+
+  return MaxPointerSize;
+}
+
 void SCCPSolver::handleCallResult(CallBase &CB) {
   Function *F = CB.getCalledFunction();
 
@@ -1333,6 +1342,27 @@
             ValueLatticeElement::getRange(NewCR, MayIncludeUndef));
         return;
       } else if (Pred == CmpInst::ICMP_EQ && CondVal.isConstant()) {
+        Constant *C = CondVal.getConstant();
+
+        // Do not propagate equality of a un-dereferenceable pointer.
+        // FIXME: Currently this only treats pointers one past the last element
+        // for array types. Should probably be much stricter.
+        if (C->getType()->isPointerTy()) {
+          BasicAAResult::DecomposedGEP Dec;
+          unsigned MaxPointerSize = getMaxPointerSize(DL);
+          Dec.StructOffset = Dec.OtherOffset = APInt(MaxPointerSize, 0);
+          Dec.StructOffset = Dec.OtherOffset = APInt(MaxPointerSize, 0);
+
+          BasicAAResult::DecomposeGEPExpression(C, Dec, DL, nullptr, nullptr);
+          auto *BaseTy = Dec.Base->getType()->getPointerElementType();
+          if (BaseTy->isArrayTy() &&
+              DL.getTypeSizeInBits(
+                  Dec.Base->getType()->getPointerElementType()) /
+                      8 ==
+                  Dec.OtherOffset) {
+            return (void)mergeInValue(IV, &CB, CopyOfVal);
+          }
+        }
         // For non-integer values or integer constant expressions, only
         // propagate equal constants.
         addAdditionalUser(OtherOp, &CB);
Index: llvm/include/llvm/Analysis/BasicAliasAnalysis.h
===================================================================
--- llvm/include/llvm/Analysis/BasicAliasAnalysis.h
+++ llvm/include/llvm/Analysis/BasicAliasAnalysis.h
@@ -104,7 +104,6 @@
   /// call site is not known.
   FunctionModRefBehavior getModRefBehavior(const Function *Fn);
 
-private:
   // A linear transformation of a Value; this class represents ZExt(SExt(V,
   // SExtBits), ZExtBits) * Scale + Offset.
   struct VariableGEPIndex {
@@ -174,6 +173,7 @@
   static bool DecomposeGEPExpression(const Value *V, DecomposedGEP &Decomposed,
       const DataLayout &DL, AssumptionCache *AC, DominatorTree *DT);
 
+private:
   static bool isGEPBaseAtNegativeOffset(const GEPOperator *GEPOp,
       const DecomposedGEP &DecompGEP, const DecomposedGEP &DecompObject,
       LocationSize ObjectAccessSize);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D85332.283304.patch
Type: text/x-patch
Size: 4018 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200805/7533ef1a/attachment.bin>