Page MenuHomePhabricator

[IR] MaxObjSize Attribute
Needs RevisionPublic

Authored by atmnpatel on Sep 19 2020, 1:44 PM.

Details

Summary

This patch add the definition for a maxobjsize attribute that keeps
track of the maximum size of the object corresponding to the pointer
value. This should help with Alias Analysis by using this as an upper
bound and dereferenceable as a lower bound on the object size, and if two
object sizes don't overlap, they cannot alias.

Diff Detail

Event Timeline

atmnpatel created this revision.Sep 19 2020, 1:44 PM
atmnpatel requested review of this revision.Sep 19 2020, 1:44 PM

I think you should split this in two patches. One that introduces the IR attribite and one for attributor deduction.

atmnpatel updated this revision to Diff 292993.Sep 19 2020, 3:46 PM

Sorry, updated accordingly.

atmnpatel retitled this revision from [Attributor][WIP] MaxObjSize Attribute to [IR] MaxObjSize Attribute.Sep 19 2020, 3:48 PM
atmnpatel edited the summary of this revision. (Show Details)

Defaults were wrong (0), switched default maxobjsize assumption (~uint64_t(0)).

efriedma added inline comments.
llvm/lib/IR/Value.cpp
724

Can you use llvm::getObjectSize here, instead of rolling your own version that has bugs?

atmnpatel added inline comments.Wed, Oct 14, 12:15 PM
llvm/lib/IR/Value.cpp
724

I don't think we can, this check here is trying to offload the actual max object size calculation to the Attributor, while getObjectSize doesn't take the attributes into account at all (from what I can tell).

efriedma added inline comments.Wed, Oct 14, 12:18 PM
llvm/lib/IR/Value.cpp
724

I mean specifically for the cases where there aren't relevant attributes, like AllocaInst and GlobalVariable.

jdoerfert added inline comments.Wed, Oct 14, 12:18 PM
llvm/lib/IR/Value.cpp
724

What @efriedma means is that you should not do the DL.getXXXXXSize lookup but instead do getObjSize which knows about corner cases.

This is the place where we only look at the IR and report properties. No analyses connections, just pure information retrieval from the IR, the "base case" for an analysis if you want.

atmnpatel updated this revision to Diff 299170.Mon, Oct 19, 2:28 PM

Sorry, I understand now. Is this more correct?

reames requested changes to this revision.Mon, Oct 19, 5:46 PM
reames added a subscriber: reames.

I have raised concerns with the way this is specified, and in particular, the interactions with dereferenceable on the llvm-dev thread. I am marking the review as "Request Changes" to ensure these comments are not lost and the patch isn't accidentally landed without addressing same.

This revision now requires changes to proceed.Mon, Oct 19, 5:46 PM