[PATCH] D66778: [Loads/SROA] Remove blatantly incorrect code and fix a bug revealed in the process

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: chandlerc, jdoerfert.
Herald added subscribers: bollu, mcrosier.
Herald added a project: LLVM.

The code we had isSafeToLoadUnconditionally was blatantly wrong.  It stripped off the constant offsets in the address, and then completely ignored them.  It proceeded to check if the original size was a valid with a zero offset on the found base pointer.  This is just completely, and utterly wrong.  Worse, the generic code just a few lines above correctly handles the cases which *are* valid.

Removing this exposes a bits vs bytes confusion in SROA + one other test change where the semantics of the test aren't clear to me.  I propose to simply change the test output (as in the diff) and move on.


Repository:
  rL LLVM

https://reviews.llvm.org/D66778

Files:
  lib/Analysis/Loads.cpp
  test/Transforms/SROA/addrspacecast.ll


Index: test/Transforms/SROA/addrspacecast.ll
===================================================================
--- test/Transforms/SROA/addrspacecast.ll
+++ test/Transforms/SROA/addrspacecast.ll
@@ -286,8 +286,10 @@
 
 define void @select_addrspacecast_gv(i1 %a, i1 %b) {
 ; CHECK-LABEL: @select_addrspacecast_gv(
-; CHECK-NEXT:    [[COND_SROA_SPECULATE_LOAD_FALSE:%.*]] = load i64, i64 addrspace(1)* @gv, align 8
-; CHECK-NEXT:    [[COND_SROA_SPECULATED:%.*]] = select i1 undef, i64 undef, i64 [[COND_SROA_SPECULATE_LOAD_FALSE]]
+; CHECK-NEXT:    [[C:%.*]] = alloca i64, align 8
+; CHECK-NEXT:    [[C_0_ASC_SROA_CAST:%.*]] = addrspacecast i64* [[C]] to i64 addrspace(1)*
+; CHECK-NEXT:    [[COND_IN:%.*]] = select i1 undef, i64 addrspace(1)* [[C_0_ASC_SROA_CAST]], i64 addrspace(1)* @gv
+; CHECK-NEXT:    [[COND:%.*]] = load i64, i64 addrspace(1)* [[COND_IN]], align 8
 ; CHECK-NEXT:    ret void
 ;
   %c = alloca i64, align 8
@@ -299,10 +301,11 @@
   ret void
 }
 
-; CHECK-LABEL: @select_addrspacecast_i8(
-; CHECK: [[SEL:%.*]] = select i1 undef, i8 undef, i8 undef
-; CHECK-NEXT: ret i8 [[SEL]]
 define i8 @select_addrspacecast_i8() {
+; CHECK-LABEL: @select_addrspacecast_i8(
+; CHECK-NEXT:    [[RET_SROA_SPECULATED:%.*]] = select i1 undef, i8 undef, i8 undef
+; CHECK-NEXT:    ret i8 [[RET_SROA_SPECULATED]]
+;
   %a = alloca i8
   %b = alloca i8
 
Index: lib/Analysis/Loads.cpp
===================================================================
--- lib/Analysis/Loads.cpp
+++ lib/Analysis/Loads.cpp
@@ -210,51 +210,12 @@
   if (isDereferenceableAndAlignedPointer(V, Align, Size, DL, CtxI, DT))
     return true;
 
-  int64_t ByteOffset = 0;
-  Value *Base = V;
-  Base = GetPointerBaseWithConstantOffset(V, ByteOffset, DL);
-
-  if (ByteOffset < 0) // out of bounds
+  if (!ScanFrom)
     return false;
-
-  Type *BaseType = nullptr;
-  unsigned BaseAlign = 0;
-  if (const AllocaInst *AI = dyn_cast<AllocaInst>(Base)) {
-    // An alloca is safe to load from as load as it is suitably aligned.
-    BaseType = AI->getAllocatedType();
-    BaseAlign = AI->getAlignment();
-  } else if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Base)) {
-    // Global variables are not necessarily safe to load from if they are
-    // interposed arbitrarily. Their size may change or they may be weak and
-    // require a test to determine if they were in fact provided.
-    if (!GV->isInterposable()) {
-      BaseType = GV->getType()->getElementType();
-      BaseAlign = GV->getAlignment();
-    }
-  }
-
+  
   PointerType *AddrTy = cast<PointerType>(V->getType());
   uint64_t LoadSize = DL.getTypeStoreSize(AddrTy->getElementType());
 
-  // If we found a base allocated type from either an alloca or global variable,
-  // try to see if we are definitively within the allocated region. We need to
-  // know the size of the base type and the loaded type to do anything in this
-  // case.
-  if (BaseType && BaseType->isSized()) {
-    if (BaseAlign == 0)
-      BaseAlign = DL.getPrefTypeAlignment(BaseType);
-
-    if (Align <= BaseAlign) {
-      // Check if the load is within the bounds of the underlying object.
-      if (ByteOffset + LoadSize <= DL.getTypeAllocSize(BaseType) &&
-          ((ByteOffset % Align) == 0))
-        return true;
-    }
-  }
-
-  if (!ScanFrom)
-    return false;
-
   // Otherwise, be a little bit aggressive by scanning the local block where we
   // want to check to see if the pointer is already being loaded or stored
   // from/to.  If so, the previous load or store would have already trapped,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66778.217284.patch
Type: text/x-patch
Size: 3551 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/659c7cbf/attachment.bin>