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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:1834
+        if (U->getType()->isPointerTy() &&
+            !CB->paramHasAttr(ArgNo, Attribute::NoCapture))
           return false;
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/GlobalStatus.cpp:164
       } 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;
----------------
This is unrelated and not clear to me.


================
Comment at: llvm/test/Transforms/GlobalOpt/localize-fnattr.ll:95
+    ret i32 %e
+}
----------------
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?


================
Comment at: llvm/test/Transforms/Mem2Reg/fnattr.ll:60
+    ret i32 %c
+}
----------------
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
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76966/new/

https://reviews.llvm.org/D76966