llvm.noalias - don't look through it to simplify GEPs

[hfinkel]

Summary

This is an important case where we shouldn't look through llvm.noalias intrinsics when stripping pointer casts; when simplifying GEPs in InstCombine. If we did, then we always end up replacing the GEP pointer argument with the (simplified) pointer argument of the llvm.noalias intrinsic. Doing that destroys the information that the noalias intrinsic is providing about the pointer value.

[majnemer]

LGTM, seems reasonable. I wonder if we should have a transform which removes redundant aliasing intrinsics...

[hfinkel]

Rebased.

[hfinkel]

Rebased.

include/llvm/IR/Value.h

#include "llvm/IR/Value.def"
   /// Returns the original pointer value.  If this is called on a non-pointer
   /// value, it returns 'this'.
   const Value *stripInBoundsConstantOffsets() const;
   Value *stripInBoundsConstantOffsets() {
     return const_cast<Value *>(
               static_cast<const Value *>(this)->stripInBoundsConstantOffsets());
   }
 
+  /// \brief Strip off pointer casts and all-zero GEPs without stripping
+  /// @llvm.noalias intrinsics.
+  ///
+  /// Returns the original uncasted value.  If this is called on a non-pointer
+  /// value, it returns 'this'.
+  const Value *stripPointerCastsExceptNoAlias() const;
+  Value *stripPointerCastsExceptNoAlias() {
+    return const_cast<Value*>(
+            static_cast<const Value *>(this)->stripPointerCastsExceptNoAlias());
+  }
+
   /// \brief Accumulate offsets from \a stripInBoundsConstantOffsets().
   ///
   /// Stores the resulting constant offset stripped into the APInt provided.
   /// The provided APInt will be extended or truncated as needed to be the
   /// correct bitwidth for an offset of this pointer type.
   ///
   /// If this is called on a non-pointer value, it returns 'this'.
   const Value *stripAndAccumulateInBoundsConstantOffsets(const DataLayout &DL,

lib/IR/Value.cpp

enum PointerStripKind {
   PSK_ZeroIndices,
   PSK_ZeroIndicesAndAliases,
   PSK_ZeroIndicesAndAliasesAndBarriers,
   PSK_InBoundsConstantIndices,
   PSK_InBounds
 };
 
 template <PointerStripKind StripKind>
-static const Value *stripPointerCastsAndOffsets(const Value *V) {
+static const Value *stripPointerCastsAndOffsets(const Value *V,
+                                                bool StripNoAlias = true) {
   if (!V->getType()->isPointerTy())
     return V;
 
   // Even though we don't look through PHI nodes, we could be called on an
   // instruction in an unreachable block, which may be on a cycle.
   SmallPtrSet<const Value *, 4> Visited;
 
   Visited.insert(V);
if (auto *GEP = dyn_cast<GEPOperator>(V)) {
       V = cast<Operator>(V)->getOperand(0);
     } else if (auto *GA = dyn_cast<GlobalAlias>(V)) {
       if (StripKind == PSK_ZeroIndices || GA->isInterposable())
         return V;
       V = GA->getAliasee();
     } else {
       if (auto CS = ImmutableCallSite(V)) {
         if (const Value *RV = CS.getReturnedArgOperand()) {
+          bool IsNoAlias = false;
+          if (auto *I = dyn_cast<IntrinsicInst>(V))
+            IsNoAlias = I->getIntrinsicID() == Intrinsic::noalias;
+
+          if (StripNoAlias || !IsNoAlias) {
             V = RV;
             continue;
           }
+        }
         // The result of invariant.group.barrier must alias it's argument,
         // but it can't be marked with returned attribute, that's why it needs
         // special case.
         if (StripKind == PSK_ZeroIndicesAndAliasesAndBarriers &&
             CS.getIntrinsicID() == Intrinsic::invariant_group_barrier) {
           V = CS.getArgOperand(0);
           continue;
         }
       }
       return V;
     }
     assert(V->getType()->isPointerTy() && "Unexpected operand type!");
   } while (Visited.insert(V).second);
 
   return V;
 }
 } // end anonymous namespace
 
 const Value *Value::stripPointerCasts() const {
   return stripPointerCastsAndOffsets<PSK_ZeroIndicesAndAliases>(this);
 }
 
+const Value *Value::stripPointerCastsExceptNoAlias() const {
+  return stripPointerCastsAndOffsets<PSK_ZeroIndicesAndAliases>(this, false);
+}
+
 const Value *Value::stripPointerCastsNoFollowAliases() const {
   return stripPointerCastsAndOffsets<PSK_ZeroIndices>(this);
 }
 
 const Value *Value::stripInBoundsConstantOffsets() const {
   return stripPointerCastsAndOffsets<PSK_InBoundsConstantIndices>(this);
 }
 

lib/Transforms/InstCombine/InstructionCombining.cpp

if (GEP.getNumIndices() == 1) {
     }
   }
 
   // We do not handle pointer-vector geps here.
   if (GEP.getType()->isVectorTy())
     return nullptr;
 
   // Handle gep(bitcast x) and gep(gep x, 0, 0, 0).
-  Value *StrippedPtr = PtrOp->stripPointerCasts();
+  Value *StrippedPtr = PtrOp->stripPointerCastsExceptNoAlias();
   PointerType *StrippedPtrTy = cast<PointerType>(StrippedPtr->getType());
 
   if (StrippedPtr != PtrOp) {
     bool HasZeroPointerIndex = false;
     if (ConstantInt *C = dyn_cast<ConstantInt>(GEP.getOperand(1)))
       HasZeroPointerIndex = C->isZero();
 
     // Transform: GEP (bitcast [10 x i8]* X to [0 x i8]*), i32 0, ...

test/Transforms/InstCombine/noalias.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+@g = global i32* null
+
+define i32 @test1(i32* %q, i32* %p, i1 %b) {
+  %pa = tail call i32* @llvm.noalias.p0i32(i32* %p, metadata !0)
+  %tmp2 = getelementptr inbounds i32, i32* %pa, i64 2
+  %val = load i32, i32* %tmp2, align 4
+  store i32 0, i32* %pa, align 4
+  store i32* %q, i32** @g, align 8
+  ret i32 %val
+
+; CHECK-LABEL: @test1
+; CHECK: call i32* @llvm.noalias.p0i32
+; CHECK: getelementptr inbounds i32, i32* %pa, i64 2
+; CHECK: ret i32
+}
+
+declare i32* @llvm.noalias.p0i32(i32*, metadata) nounwind
+
+!0 = !{!0, !"some domain"}
+!1 = !{!1, !0, !"some scope"}