Thanks Nick and Chandler for the reviews! Since this patch touches a
lot of different places, I definitely want to make sure I don't break
(too many) things.
I send a new patch in attach, which I believe addresses all concerns
raised so far.
> I think this should really be two pieces, one which determines the size of
> things which can be done without modifying the code and a wrapper around
> that which builds up IR when that fails. Here's roughly what I have in mind:
Well, I gave it a try and I split the code in the new patch.
The only thing that bothers me is that there is a little code
duplication. But I agree the code becomes much cleaner.
> Index: lib/Analysis/IPA/GlobalsModRef.cpp
> ===================================================================
> --- lib/Analysis/IPA/GlobalsModRef.cpp (revision 158666)
> +++ lib/Analysis/IPA/GlobalsModRef.cpp (working copy)
> @@ -329,15 +329,8 @@
> // Check the value being stored.
> Value *Ptr = GetUnderlyingObject(SI->getOperand(0));
>
> - if (isMalloc(Ptr)) {
> - // Okay, easy case.
> - } else if (CallInst *CI = dyn_cast<CallInst>(Ptr)) {
> - Function *F = CI->getCalledFunction();
> - if (!F || !F->isDeclaration()) return false; // Too hard to
> analyze.
> - if (F->getName() != "calloc") return false; // Not calloc.
> - } else {
> + if (!isNoAliasFn(Ptr))
> return false; // Too hard to analyze.
> - }
>
> I'm not sure this is right.. There's another assumption that globals
> mod-ref is making which isn't in the comments; it's assuming that the
> called functions (malloc and calloc) have no side-effects other than the
> allocation of memory. In general, the callee needs to mark
> "FunctionInfo[Writers[i]].GlobalInfo[I] |= Mod" when isNoAlias but can
> remove that marking when it really is malloc/calloc.
You're right. I replaced that call with isAllocLikeFn().
> +enum AllocType {
> + MallocLike = 1<<0, // allocates
> + CallocLike = 1<<1, // allocates + bzero
> + ReallocLike = 1<<2, // reallocates
> + StrDupLike = 1<<3,
> + AllocLike = MallocLike | CallocLike | StrDupLike,
> + AnyAlloc = MallocLike | CallocLike | ReallocLike | StrDupLike
> +};
>
> I don't understand why StrDup gets special handling? "fopen" also returns
> newly allocated memory. What they have in common is the noalias marker. Are
> you using this like calloc to indicate that the memory is initialized to a
> copy of other memory? Please add comments.
strdup() has to be treated separately because the function to compute
the object's size (strlen(str)+1) has a different pattern from the
others (which is a simple multiplication of up to 2 parameters).
Supporting fopen() here is not very important IMHO, since client code
will not dereference a FILE* directly, so the information about its
size is probably not very important. For alias analysis purposes, the
function should be marked noalias.
Thanks,
Nuno
>
>
> Nick
>
> On 18 June 2012 14:19, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>
>> Hi,
>>
>> Right now, there are multiple implementations of analysis of memory
>> allocation functions across the LLVM codebase, including:
>> - computing size of pointed objects (dead store elimination, instcombine,
>> basic alias analysis)
>> - recognize allocation functions (e.g., to know that the return value
>> doesn't alias anything else, or to recognize uninitialized allocated memory
>> for undef purposes).
>>
>> Therefore I send in attach a patch to refactor the MemoryBuiltin Analysis
>> to include all the functionality needed by the other passes just mentioned.
>> It removes a lot of code duplication, and the proposed implementation is
>> better than all the previous ones combined. The patch adds support for the
>> noalias function attribute as well.
>> This patch also adds support for computing the size of pointed objects at
>> run-time, which is basically a copy from the bounds checking pass. After
>> this patch goes in, most of the code in the bounds checking pass can be
>> removed.
>>
>> Any comments/suggestions/etc..?
>>
>> Thanks,
>> Nuno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_memorybuiltin2.diff
Type: text/x-patch
Size: 45532 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120620/383c192a/attachment.bin>