[CFLGraph] Change isMallocOrCallocLikeFn to isNoAliasCall now that allocation functions must be marked noalias

[Bryce-MW]

Summary

Allocation functions must now be marked noalias and any function marked noalias does not introduce aliases. The if condition below like 461 will now always be true because Fn->returnDoesNotAlias() returns the same result as isNoAliasCall(&Call). Since we return early if isNoAliasCall returns true, it will always be false at the point of the condition. I'm happy to restore that condition for safety if wanted.

[Bryce-MW]

Hmm, I expected that Herald would add other reviewers automatically. I'm not sure if I can ask it to do that manually

[Bryce-MW]

• Rebase to fix failing test on Windows

[xbolva00]

Testcase?

[nikic]

I'm not familiar with CFL, but I don't think this was the intention of the code. I think this part tries to reason about inaccessiblememonly/inaccessibleorargmemonly in a clumsy way.

[reames]

I agree with @nikic on the intent of the code. This can probably be replaced with a check for inaccessiblememonly instead.

The change as implemented is incorrect. There is no requirement that an noalias call not have other side effects. Example:
define nolias i8* helper(i64 %size, i8* %unrelated) {

%res = call i8* malloc(i64 %size)
store i8 0, i8* %unrelated
ret %res

}

Thanks for looking at this!

[Bryce-MW]

Thanks for catching that. I'm glad we have review. For a normal function, it tries interprocedural analysis, if that fails, it looks at the readonly and noalias attributes. I think that allocation functions can go through the same process but just also check Call.onlyAccessesInaccessibleMemory(). This would mean that allocation functions could undergo interprocedural analysis first which would be a functionality change from before.

[Bryce-MW]

• Use onlyAccessesInaccessibleMemory instead

[reames]

I went and took a slightly deeper look at this code, and unfortunately, I think we have a couple of interacting issues here we will need to fix if we want to remove the isMallocOrCallocLike check.

Issue 1 - The inter-procedural inference code does not appear to restrict results based on call site attributes. It appears to only look at function attributes. This means that a call site which is annotated inaccessiblememonly to a call which isn't would get the conservative answer, not the precise one.

Issue 2 - The inter-procedural inference code (getAliasSummary) does not appear to incorporate function attributes into the result at all. (I may be misreading this code, it's horribly "generic" and really hard to figure out what it is actually doing.)

Issue 3 - The cumulative result of the previous two is that letting noalias only trigger if inter-procedural doesn't is a regression.

I think what I'd suggest is that rather than removing this code, we focus on simple generalizing it.

I believe the property isMallocOrCallLike is actually checking for here is: a noalias function whose only side effect is inaccessiblememonly. We can generalize the check - with added test coverage - and at least remove the overly specific call.

[nikic]

It may be worth considering dropping the CFL code entirely. It's not used in a default pipeline, and I rather doubt that this is ever going to change.

llvm/lib/Analysis/CFLGraph.h

public:
     void visitCallBase(CallBase &Call) {
       // Make sure all arguments and return value are added to the graph first
       for (Value *V : Call.args())
         if (V->getType()->isPointerTy())
           addNode(V);
       if (Call.getType()->isPointerTy())
         addNode(&Call);
 
-      // Check if Inst is a call to a library function that
-      // allocates/deallocates on the heap. Those kinds of functions do not
-      // introduce any aliases.
-      // TODO: address other common library functions such as realloc(),
-      // strdup(), etc.
-      if (isMallocOrCallocLikeFn(&Call, &TLI) || isFreeCall(&Call, &TLI))
+      // Check if Inst is a call to a library function that frees memory. Those
+      // kinds of functions do not introduce any aliases.
+      if (isFreeCall(&Call, &TLI))
         return;
 
       // TODO: Add support for noalias args/all the other fun function
       // attributes that we can tack on.
       SmallVector<Function *, 4> Targets;
       if (getPossibleTargets(Call, Targets))
         if (tryInterproceduralAnalysis(Call, Targets))
           return;
 
       // Because the function is opaque, we need to note that anything
       // could have happened to the arguments (unless the function is marked
       // readonly or readnone), and that the result could alias just about
       // anything, too (unless the result is marked noalias).
-      if (!Call.onlyReadsMemory())
+      if (!(Call.onlyReadsMemory() || Call.onlyAccessesInaccessibleMemory()))
         for (Value *V : Call.args()) {
           if (V->getType()->isPointerTy()) {
             // The argument itself escapes.
             Graph.addAttr(InstantiatedValue{V, 0}, getAttrEscaped());
             // The fate of argument memory is unknown. Note that since
             // AliasAttrs is transitive with respect to dereference, we only
             // need to specify it for the first-level memory.
             Graph.addNode(InstantiatedValue{V, 1}, getAttrUnknown());