[InstComineLoadStoreAlloca] Optimize stores to GEP off null base

[anna]

Summary

Currently, in InstCombineLoadStoreAlloca, we have simplification rules
for the following cases:

1. load off a null
2. load off a GEP with null base
3. store to a null

This patch adds support for the fourth case which is store into a GEP with null
base. Since this is UB as well (and directly analogous to the load off a
GEP with null base), we can substitute the stored val with
undef in instcombine, so that SimplifyCFG can optimize this code into
unreachable code.

Note: Right now, simplifyCFG hasn't been taught about optimizing this to
unreachable and adding an llvm.trap (this is already done for the above 3 cases).

I would like to confirm whether this is just a missed optimization, or
there is something fundamental that prevents this transformation.

[davide]

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

941–943

return isa<> ?

[sanjoy]

lgtm

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

935

Minor bikeshed -- how about checking SI.getPointerAddressSpace() == 0 with an early exit?

937

How about:

if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(Ptr))
  Ptr = GEPI->getOperand(0);
// Logic for Ptr here

?

[davide]

Forgot to click accept

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

if (canReplaceGEPIdxWithZero(IC, GEPI, &MemI, Idx)) {
       MemI.setOperand(MemI.getPointerOperandIndex(), NewGEPI);
       return NewGEPI;
     }
   }
 
   return nullptr;
 }
 
+static bool canSimplifyNullStoreOrGEP(StoreInst &SI) {
+  auto *Ptr = SI.getPointerOperand();
+  if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(Ptr)) {
+    const Value *GEPI0 = GEPI->getOperand(0);
+    if (isa<ConstantPointerNull>(GEPI0) && GEPI->getPointerAddressSpace() == 0)
+      return true;
+  }
+  if (isa<ConstantPointerNull>(Ptr) && SI.getPointerAddressSpace() == 0)
+    return true;
+  return false;
+}
+
 static bool canSimplifyNullLoadOrGEP(LoadInst &LI, Value *Op) {
   if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(Op)) {
     const Value *GEPI0 = GEPI->getOperand(0);
     if (isa<ConstantPointerNull>(GEPI0) && GEPI->getPointerAddressSpace() == 0)
       return true;
   }
   if (isa<UndefValue>(Op) ||
       (isa<ConstantPointerNull>(Op) && LI.getPointerAddressSpace() == 0))
for (unsigned ScanInsts = 6; BBI != SI.getParent()->begin() && ScanInsts;
     }
 
     // Don't skip over loads, throws or things that can modify memory.
     if (BBI->mayWriteToMemory() || BBI->mayReadFromMemory() || BBI->mayThrow())
       break;
   }
 
   // store X, null    -> turns into 'unreachable' in SimplifyCFG
-  if (isa<ConstantPointerNull>(Ptr) && SI.getPointerAddressSpace() == 0) {
+  // store X, GEP(null, Y) -> turns into 'unreachable' in SimplifyCFG
+  if (canSimplifyNullStoreOrGEP(SI)) {
     if (!isa<UndefValue>(Val)) {
       SI.setOperand(0, UndefValue::get(Val->getType()));
       if (Instruction *U = dyn_cast<Instruction>(Val))
         Worklist.Add(U);  // Dropped a use.
     }
     return nullptr;  // Do not modify these!
   }
 

test/Transforms/InstCombine/store.ll

define void @test2(i32* %P) {
         %X = load i32, i32* %P               ; <i32> [#uses=1]
         %Y = add i32 %X, 0              ; <i32> [#uses=1]
         store i32 %Y, i32* %P
         ret void
 ; CHECK-LABEL: @test2(
 ; CHECK-NEXT: ret void
 }
 
+define void @store_at_gep_off_null(i64 %offset) {
+; CHECK-LABEL: @store_at_gep_off_null
+; CHECK: store i32 undef, i32* %ptr
+   %ptr = getelementptr i32, i32 *null, i64 %offset
+   store i32 24, i32* %ptr
+   ret void
+}
+
 ;; Simple sinking tests
 
 ; "if then else"
 define i32 @test3(i1 %C) {
   %A = alloca i32
         br i1 %C, label %Cond, label %Cond2
 
 Cond: