Page MenuHomePhabricator

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

Authored by ddcc on Mar 27 2020, 6:49 PM.

Details

Reviewers
jdoerfert
Summary

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

Depends on: D76894, D76965

Diff Detail

Event Timeline

ddcc created this revision.Mar 27 2020, 6:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 6:49 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
ddcc added a comment.Mar 27 2020, 6:54 PM

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 added inline comments.Mar 27 2020, 11:41 PM
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 marked 2 inline comments as done.Mar 28 2020, 3:43 PM

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.