[GlobalOpt/GlobalStatus][Mem2Reg] Handle PtrToInt passed as function call operand

[ddcc]

Summary

Support optimizing calls to readnone function with ptrtoint operand in global variable optimization and mem2reg

Depends on: D76894, D76965

[ddcc]

I'm not quite sure what the intended semantics for inaccessiblememonly and readnone are here. If a pointer operand is converted to an integer and passed as an argument to a function call, when can that pointer be optimized out? Does that function need to have either attribute, or does it not matter that an integer argument is casted from a pointer? This patch currently assumes that the attribute must still be present on the function (since it can't be set on a non-pointer argument).

[jdoerfert]

llvm/lib/Transforms/IPO/GlobalOpt.cpp

1834

This is not sound. ptr2int + call can capture the pointer just fine so any use here has to be assumed capturing. That said, I would strongly recommend against looking through ptr2int if you don't have a good reason and good reasoning for soundness.

---

I looked at your tests, it seems you really want to support ptr2int. I see two options:

1. Allow nocapture for non-pointer arguments, or
2. Perform the sufficient nocapture check here if it is in integer. Since I don't want to duplicate the logic we should instead put it in a helper. Take a look at determineFunctionCaptureCapabilities in Attributor.cpp. The first check is to rule out capturing. We can make it a static member of AANoCapture in Attributor.h so we can call it here. Other places are fine too.

llvm/lib/Transforms/Utils/GlobalStatus.cpp

164

This is unrelated and not clear to me.

llvm/test/Transforms/GlobalOpt/localize-fnattr.ll

95

I'm curious, what value is passed to @foo5 here? An alloca? What if foo5 does something like:

void foo5(int a) {
  assert(a == &G5);
}

If that can't happen, what if we pass @G5 twice?

llvm/test/Transforms/Mem2Reg/fnattr.ll

60

FWIW, the test case to break the current code looks like this:

define i32* toptr(i32 %a) readnone nowunind {
  %ptr = inttoptr i32 %a, i32*
  ret %ptr
}

; Do read from casted pointer argument
define i32 @c() norecurse {
    %g3 = alloca i32
    store i32 42, i32 *%g3
    %p = ptrtoint i32* %g3 to i32
    %clone = call i32* @toptr(i32 %p)
    store i32 1337, i32* %clone
    %c = load i32, i32* %g3
    ret i32 %c
}

[ddcc]

I'm not sure yet if I'll need to support ptrtoint; I haven't rebuilt my benchmarks with the parent changes to see if optimizations are being inhibited. But I am more inclined to avoid it if possible since I'll need to refactor and call the code to infer pointer capture.

llvm/test/Transforms/GlobalOpt/localize-fnattr.ll

95

I don't believe this can happen, because the path from processGlobal() -> processInternalGlobal() -> isPointerValueDeadOnEntryToFunction() checks that the global has internal linkage and does not have multiple accessing functions.

When the localized alloca of @G5 is passed in twice, they should compare equal.

llvm/test/Transforms/Mem2Reg/fnattr.ll

60

Ok, yeah, the capture check would be needed for this case.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

static bool isPointerValueDeadOnEntryToFunction(
   SmallVector<std::pair<const Instruction *, Type *>, 4> Stores;
   SmallVector<Value *, 4> Roots;
 
   Roots.push_back(GV);
   while (Roots.size()) {
     Value *R = Roots.pop_back_val();
     for (auto &U : R->uses()) {
       User *UU = U.getUser();
-      if (Operator::getOpcode(UU) == Instruction::BitCast)
+      const unsigned OpCode = Operator::getOpcode(UU);
+      if (OpCode == Instruction::BitCast || OpCode == Instruction::PtrToInt)
         Roots.push_back(UU);
       else if (auto *LI = dyn_cast<LoadInst>(UU))
         Loads.push_back(std::make_pair<>(LI, LI->getType()));
       else if (auto *SI = dyn_cast<StoreInst>(UU))
         Stores.push_back(
             std::make_pair<>(SI, SI->getValueOperand()->getType()));
       else if (auto *CB = dyn_cast<CallBase>(UU)) {
         // TODO: Handle isDroppable() case
         if (!CB->isArgOperand(&U))
           return false;
         unsigned ArgNo = CB->getArgOperandNo(&U);
         // Argument must not be captured for subsequent use
-        if (!CB->paramHasAttr(ArgNo, Attribute::NoCapture))
+        if (U->getType()->isPointerTy() &&
+            !CB->paramHasAttr(ArgNo, Attribute::NoCapture))
           return false;
         // Depending on attributes, either treat calls as loads/stores at the
         // call site, or ignore them if they are not going to be dereferenced.
         if (CB->hasFnAttr(Attribute::InaccessibleMemOnly) ||
             CB->hasFnAttr(Attribute::ReadNone) ||
             CB->paramHasAttr(ArgNo, Attribute::ReadNone))
           continue;
         else if (CB->hasFnAttr(Attribute::ReadOnly) ||

llvm/lib/Transforms/Utils/GlobalStatus.cpp

if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(V))
     if (GV->isExternallyInitialized())
       GS.StoredType = GlobalStatus::StoredOnce;
 
   for (const Use &U : V->uses()) {
     const User *UR = U.getUser();
     if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(UR)) {
       GS.HasNonInstructionUser = true;
 
-      // If the result of the constantexpr isn't pointer type, then we won't
-      // know to expect it in various places.  Just reject early.
-      if (!isa<PointerType>(CE->getType()))
+      // If the result of the constantexpr isn't pointer/integer type, then we
+      // won't know to expect it in various places.  Just reject early.
+      if (!isa<PointerType>(CE->getType()) && !isa<IntegerType>(CE->getType()))
         return true;
 
       // FIXME: Do we need to add constexpr selects to VisitedUsers?
       if (analyzeGlobalAux(CE, GS, VisitedUsers))
         return true;
     } else if (const Instruction *I = dyn_cast<Instruction>(UR)) {
       if (!GS.HasMultipleAccessingFunctions) {
         const Function *F = I->getParent()->getParent();
if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(UR)) {
               // noop.
             } else {
               GS.StoredType = GlobalStatus::Stored;
             }
           } else {
             GS.StoredType = GlobalStatus::Stored;
           }
         }
-      } else if (isa<BitCastInst>(I) || isa<GetElementPtrInst>(I)) {
+      } else if (isa<BitCastInst>(I) || isa<GetElementPtrInst>(I) ||
+                 isa<PtrToIntInst>(I)) {
         // Skip over bitcasts and GEPs; we don't care about the type or offset
         // of the pointer.
         if (analyzeGlobalAux(I, GS, VisitedUsers))
           return true;
       } else if (isa<SelectInst>(I) || isa<PHINode>(I)) {
         // Look through selects and PHIs to find if the pointer is
         // conditionally accessed. Make sure we only visit an instruction
         // once; otherwise, we can get infinite recursion or exponential
         // compile time.
         if (VisitedUsers.insert(I).second)
           if (analyzeGlobalAux(I, GS, VisitedUsers))
             return true;
       } else if (isa<CmpInst>(I)) {
         GS.IsCompared = true;
       } else if (const MemTransferInst *MTI = dyn_cast<MemTransferInst>(I)) {
         if (MTI->isVolatile())
           return true;
         if (MTI->getArgOperand(0) == V)
           GS.StoredType = GlobalStatus::Stored;
         if (MTI->getArgOperand(1) == V)
           GS.IsLoaded = true;
       } else if (const MemSetInst *MSI = dyn_cast<MemSetInst>(I)) {
-        assert(MSI->getArgOperand(0) == V && "Memset only takes one pointer!");
-        if (MSI->isVolatile())
+        if (MSI->isVolatile() || MSI->getArgOperand(0) != V)
           return true;
         GS.StoredType = GlobalStatus::Stored;
       } else if (const CallBase *CB = dyn_cast<CallBase>(I)) {
         if (CB->isCallee(&U)) {
           GS.IsLoaded = true;
         } else if (CB->isArgOperand(&U)) {
           unsigned ArgNo = CB->getArgOperandNo(&U);
           // Argument must not be captured for subsequent use
-          if (!CB->paramHasAttr(ArgNo, Attribute::NoCapture))
+          if (U->getType()->isPointerTy() &&
+              !CB->paramHasAttr(ArgNo, Attribute::NoCapture))
             return true;
           // Depending on attributes, treat the operand as a pure call, load, or
           // store at the call site.
           if (CB->hasFnAttr(Attribute::InaccessibleMemOnly) ||
               CB->hasFnAttr(Attribute::ReadNone) ||
               CB->paramHasAttr(ArgNo, Attribute::ReadNone))
             continue;
           else if (CB->hasFnAttr(Attribute::ReadOnly) ||

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp

for (const Use &UU : I->uses()) {
           return false; // Don't allow a store OF the AI, only INTO the AI.
         // Note that atomic stores can be transformed; atomic semantics do
         // not have any meaning for a local alloca.
         if (SI->isVolatile())
           return false;
       } else if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(U)) {
         if (!II->isLifetimeStartOrEnd())
           return false;
+      } else if (const PtrToIntInst *PTI = dyn_cast<PtrToIntInst>(U)) {
+        Roots.push_back(PTI);
       } else if (const BitCastInst *BCI = dyn_cast<BitCastInst>(U)) {
         Roots.push_back(BCI);
       } else if (const GetElementPtrInst *GEPI =
                      dyn_cast<GetElementPtrInst>(U)) {
         if (GEPI->getType() != Type::getInt8PtrTy(U->getContext(), AS))
           return false;
         if (!GEPI->hasAllZeroIndices())
           return false;
while (Roots.size()) {
     Visited.insert(I);
     for (auto UI = I->use_begin(), UE = I->use_end(); UI != UE;) {
       Instruction *U = cast<Instruction>(UI->getUser());
       unsigned ArgNo =
           isa<CallBase>(U) ? cast<CallBase>(U)->getArgOperandNo(&*UI) : -1;
       ++UI;
       if (isa<LoadInst>(U) || isa<StoreInst>(U))
         continue;
-      else if (isa<BitCastInst>(U) || isa<GetElementPtrInst>(U))
+      else if (isa<BitCastInst>(U) || isa<GetElementPtrInst>(U) ||
+               isa<PtrToIntInst>(U))
         Roots.push_back(U);
       else if (CallBase *CB = dyn_cast<CallBase>(U)) {
         if (isa<IntrinsicInst>(U))
           U->eraseFromParent();
         else {
           // Replace this operand with an UndefValue because it is readnone.
           CB->setArgOperand(ArgNo, UndefValue::get(I->getType()));
         }

llvm/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca2.ll

 ; Make sure we don't break the IR when moving non-instrumented allocas
 
 ; RUN: opt < %s -asan -asan-module -S | FileCheck %s
 ; RUN: opt < %s -asan -asan-module -asan-instrument-dynamic-allocas -S | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.10.0"
 
+declare void @bar(i32)
+
 define i32 @foo() sanitize_address {
 entry:
   %non_instrumented1 = alloca i32, align 4
   %t = load i32, i32* %non_instrumented1, align 4
   %instrumented = alloca i32, align 4
   %ptr = ptrtoint i32* %instrumented to i32
+  call void @bar(i32 %ptr)
   ret i32 %t
 }
 
 ; CHECK: entry:
 ; CHECK: %non_instrumented1 = alloca i32, align 4
 ; CHECK: load i32, i32* %non_instrumented1
 ; CHECK: load i32, i32* @__asan_option_detect_stack_use_after_return

llvm/test/Transforms/GlobalOpt/localize-fnattr.ll

; CHECK: }
     store i32 42, i32 *@G4
     call void @llvm.assume(i1 true) ["align"(i32 *@G4, i64 128)]
     %p = bitcast i32* @G4 to i8*
     call void @foo4a(i8* %p, i8 0)
     call void @foo4b(i8* %p, i8 0)
     %d = load i32, i32* @G4
     ret i32 %d
 }
+
+declare void @foo5(i32, i8) local_unnamed_addr readnone
+
+@G5 = internal global i32 0
+
+; Doesn't read from casted pointer argument
+define i32 @e() norecurse {
+; CHECK-LABEL: @e
+; CHECK: alloca
+; CHECK-NOT: @G5
+; CHECK: }
+    store i32 42, i32 *@G5
+    %p = ptrtoint i32* @G5 to i8
+    call void @foo5(i32 ptrtoint (i32* @G5 to i32), i8 %p)
+    %e = load i32, i32* @G5
+    ret i32 %e
+}

llvm/test/Transforms/Mem2Reg/fnattr.ll

; CHECK: }
     %g2 = alloca i32
     store i32 42, i32 *%g2
     %p = bitcast i32* %g2 to i8*
     call void @foo2a(i8* %p, i8 0)
     call void @foo2b(i8* %p, i8 0)
     %b = load i32, i32* %g2
     ret i32 %b
 }
+
+declare void @foo3(i32, i8) local_unnamed_addr readnone
+
+; Doesn't read from casted pointer argument
+define i32 @c() norecurse {
+; CHECK-LABEL: @c
+; CHECK-NOT: alloca
+; CHECK-NOT: ptrtoint
+; CHECK-NOT: %g3
+; CHECK: undef
+; CHECK: ret i32 42
+; CHECK: }
+    %g3 = alloca i32
+    store i32 42, i32 *%g3
+    %p = ptrtoint i32* %g3 to i32
+    call void @foo3(i32 %p, i8 0)
+    %c = load i32, i32* %g3
+    ret i32 %c
+}