This is an archive of the discontinued LLVM Phabricator instance.

Basic support for posix_memalign / __builtin_object_size interaction
ClosedPublic

Authored by serge-sans-paille on Mar 24 2022, 1:42 PM.

Details

Summary

It actually implements support for seeing through loads, using alias analysis to
refine the result.

This is rather limited, but I didn't want to rely on more than available analysis at that point (to be gentle with compilation time), and it does seem to catch some common scenario, as showcased by the included tests.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 1:42 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
serge-sans-paille requested review of this revision.Mar 24 2022, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 1:42 PM
xbolva00 added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
156

why remove? no record above for posix_memalign was added..

This is pretty unusual in terms of approach, it would be more typical to do a limited upward walk to find the clobbering store (a la FindAvailableLoadedValue).

llvm/lib/Analysis/MemoryBuiltins.cpp
839

What about the case where the object size is taken prior to initialization? In the alloca case it will be an undef initialization -- is it fine to simply ignore that case? I'd probably expect min at least to return zero for that.

Probably more obviously problematic is the noalias case, because that one doesn't make any statement on the initial contents of the memory. For a typical allocator that will be either undef or null, but this is not a requirement of noalias (an obvious violation is strdup).

901

I don't get what this check is supposed to do. You're asking here whether an escaping pointer and an escape-source alias, for which the answer should always be "yes" for the SimpleCaptureInfo-based AA used here. Can you give an example where this check does something useful?

nikic added inline comments.Mar 25 2022, 1:50 AM
llvm/lib/Analysis/MemoryBuiltins.cpp
905

To double check, does this work when a non-pointer type is stored? You might want to add a test that stores an i32 or so into a bitcasted pointer.

Following @nikic suggestion, use a CFG-based graph transversal à la FindAvailableLoadedValue

Added test case for simple load interactions.

Take into account posix_memalign return value, as suggested by @nikic

xbolva00 added inline comments.Apr 5 2022, 6:03 AM
llvm/lib/Analysis/MemoryBuiltins.cpp
156

Still no answer

891

Do you need FakeCond at all?

llvm::isImpliedByDomCondition (CmpInst::Predicate Pred, const Value *LHS, const Value *RHS, const Instruction *ContextI, const DataLayout &DL)

serge-sans-paille added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
156

Yup, sorry: because it's API is so different (and unique ?) from other library functions. It doesn't fit any of the existing MallocFamily nor AllocType.

891

Indeed!

serge-sans-paille marked an inline comment as done.Apr 5 2022, 6:46 AM
nikic added inline comments.Apr 5 2022, 6:59 AM
llvm/include/llvm/Analysis/MemoryBuiltins.h
179

Make it an optional parameter instead?

llvm/lib/Analysis/MemoryBuiltins.cpp
586

Can assign unconditionally.

820

With the getUniquePredecessor() variant noted before this wouldn't matter, but if you do explore branching cases, then I'm not sure returning unknown here makes sense. Consider a simple store; if (...) { ... } else { ... } load; case. The upwards walk would hit the store along one path first and then along the other again and return unknown at that pointer.

823

This should probably only be counted once on the top level?

828

We should be checking against a budget here, to avoid scanning 150k instructions in search of a matching store...

835

Use getPointerOperand() here.

Also probably need to check SI->getValueOperand()->getType()->isPointerTy(), unless compute() handles non-pointer values gracefully.

840

This should be checking aliasing with Load.getPointerOperand(), *not* &Load itself.

887

bos -> the load

908

Should queue on a worklist rather than recursing. For the posix_memalign use-case, it might make sense to just restrict to getUniquePredecessor(), as we probably don't care about branching cases there?

serge-sans-paille marked 5 inline comments as done.

take @nikic remarks into account.

llvm/include/llvm/Analysis/MemoryBuiltins.h
179

That's a choice: I wanted to keep it grouped with TLI so that all analyse are kept next to each other.

llvm/lib/Analysis/MemoryBuiltins.cpp
823

This is used to count the "Number of load instructions with unsolved size and offset", thus the detailed counting.

835

I *think* this is already guarenteed by the fact that we match against the load pointer operand, which is of type T**

nikic added inline comments.Apr 6 2022, 4:16 AM
llvm/lib/Analysis/MemoryBuiltins.cpp
820

I think this limit is still overly large -- I think our usual value starting value for limits like this is 32. If you think that's too low to be practically useful, I'd go with 128 here.

823

Right, but currently you're incrementing this once per unknown predecessor, rather than once per load, no?

835

This isn't guaranteed with opaque pointers -- but also for the more general MustAlias case below, which will strip pointer casts.

837

Should have a if(I.isDebugInst()) continue check here, to make sure debug insts can't affect optimization. (Before the limit check.)

846

Just wondering if it might be better to just bail entirely from the load-handling code if we don't have AA, rather than having extra code to support both? I don't particularly mind, but given how we essentially only care about the one lowerObjectSizeCall() usage that does have AA, this may not be particularly useful. We'd have to add some tests for a different caller that goes through these non-AA codepaths.

859

nit: getValueOperand()

871

Second part of this comment is now outdated :)

889

nit: Load.getPointerOperand()

923

As a small optimization, we could short-circuit on unknown here.

serge-sans-paille marked an inline comment as done.

Another review round trip.

nikic accepted this revision.Apr 6 2022, 5:57 AM

LGTM

llvm/lib/Analysis/MemoryBuiltins.cpp
898

Should be Unknown().

llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-load.ll
3

We should pass in the AA parameter in LowerConstantIntrinsics to avoid the need for -instcombine here, but we can do that separately...

This revision is now accepted and ready to land.Apr 6 2022, 5:57 AM
This revision was landed with ongoing or failed builds.Apr 8 2022, 12:32 AM
This revision was automatically updated to reflect the committed changes.