[CFLAA] Try to be less conservative on more functions

[grievejia]

Summary

I've changed the criteria of marking a function f external from "f->hasLocalLinkage()" to "f->isInterposable()". I think the former is overly conservative since it only handles functions with internal or private linkage type, yet all we want for non-external functions is that they can't be overwritten while linking. If my thought turns out to be wrong, I'm happy to switch it back.

[sanjoy]

Drop by comment

lib/Analysis/CFLAliasAnalysis.cpp

371 ↗

(On Diff #61231)

If this is doing inter-procedural alias analysis, then you need to use Fn-> hasExactDefinition().

[grievejia]

Change isInterposable() to !hasExactDefinition()

lib/Analysis/CFLAliasAnalysis.cpp

371 ↗

(On Diff #61231)

Thank you so much for pointing that out!

[sanjoy]

lib/Analysis/CFLAliasAnalysis.cpp

371 ↗

(On Diff #61251)

hasExactDefinition checks for isDeclaration. You should just be able to replace isFunctionExternal(F) with F->hasExactDefinition().

[grievejia]

Remove Fn->isDeclaration().

Also, a test xfail was just discovered in the dependent patch. It could be removed in this patch, as the diff shows.

[grievejia]

lib/Analysis/CFLAliasAnalysis.cpp

380 ↗

(On Diff #61262)

I think I'll just keep isFunctionExternal() here, in case other alias analysis-specific checks are needed.

[george.burgess.iv]

I'm assuming Sanjoy's fine with this, so LGTM -- thanks for splitting this out.

[sanjoy]

In D21513#462355, @george.burgess.iv wrote:

I'm assuming Sanjoy's fine with this, so LGTM -- thanks for splitting this out.

Yes, this looks fine.

[george.burgess.iv]

It seems that applying this patch on top of D21475 causes test/Analysis/CFLAliasAnalysis/basic-interproc-ret.ll to fail.

The output I get from the test suite is:

test/Analysis/CFLAliasAnalysis/basic-interproc-ret.ll:8:10: error: expected string not found in input
; CHECK: 3 no alias responses
         ^
<stdin>:5:2: note: scanning from here
 4 no alias responses (100.0%)
 ^

And the output I get from running the test directly is

Function: test2: 2 pointers, 0 call sites
Function: test: 3 pointers, 1 call sites
===== Alias Analysis Evaluator Report =====
  4 Total Alias Queries Performed
  4 no alias responses (100.0%)
  0 may alias responses (0.0%)
  0 partial alias responses (0.0%)
  0 must alias responses (0.0%)
  Alias Analysis Evaluator Pointer Alias Summary: 100%/0%/0%/0%
  3 Total ModRef Queries Performed
  1 no mod/ref responses (33.3%)
  0 mod responses (0.0%)
  0 ref responses (0.0%)
  2 mod & ref responses (66.6%)
  Alias Analysis Evaluator Mod/Ref Summary: 33%/0%/0%/66%

Is this intentional? (If so, no need to re-upload the diff; I can just tweak the test to expect 4 NoAlias responses)

[grievejia]

Interesting. I remembered that I deleted this prticular test case in the previous patch (what it tests has already been covered by other cases). It is likely that I messed up something and the removal was not included. I'm ok with either a fix or a removal of it.

Thank you so much for bringing this up, by the way!

llvm/trunk/lib/Analysis/CFLAliasAnalysis.cpp

void visitVAArgInst(VAArgInst &Inst) {
     //  1. Loads a value from *((T*)*Ptr).
     //  2. Increments (stores to) *Ptr by some target-specific amount.
     // For now, we'll handle this like a landingpad instruction (by placing the
     // result in its own group, and having that group alias externals).
     addNodeWithAttr(&Inst, AttrUnknown);
   }
 
   static bool isFunctionExternal(Function *Fn) {
-    return Fn->isDeclaration() || !Fn->hasLocalLinkage();
+    return !Fn->hasExactDefinition();
   }
 
   bool tryInterproceduralAnalysis(CallSite CS,
                                   const SmallVectorImpl<Function *> &Fns) {
     assert(Fns.size() > 0);
 
     if (CS.arg_size() > MaxSupportedArgsInSummary)
       return false;

llvm/trunk/test/Analysis/CFLAliasAnalysis/basic-interproc-ret.ll

-; This testcase ensures that CFL AA gives conservative answers on variables
-; that involve arguments.
-
-; RUN: opt < %s -cfl-aa -aa-eval -print-may-aliases -disable-output 2>&1 | FileCheck %s
-
-; CHECK:     Function: test
-; CHECK: 4 Total Alias Queries Performed
-; CHECK: 3 no alias responses
-; ^ The 1 MayAlias is due to %arg1. Sadly, we don't currently have machinery
-; in place to check whether %arg1 aliases %a, because BasicAA takes care of 
-; that for us.
-
-define i32* @test2(i32* %arg1) {
-  store i32 0, i32* %arg1
-
-  %a = alloca i32, align 4
-  ret i32* %a
-}
-
-define void @test() {
-  %a = alloca i32, align 4
-  %b = alloca i32, align 4
-  %c = call i32* @test2(i32* %a)
-
-  ret void
-}

llvm/trunk/test/Analysis/CFLAliasAnalysis/interproc-ret-arg.ll

 ; This testcase ensures that CFL AA answers queries soundly when callee tries 
 ; to return one of its parameters
 
 ; RUN: opt < %s -disable-basicaa -cfl-aa -aa-eval -print-all-alias-modref-info -disable-output 2>&1 | FileCheck %s
 ; RUN: opt < %s -aa-pipeline=cfl-aa -passes=aa-eval -print-all-alias-modref-info -disable-output 2>&1 | FileCheck %s
 
-; We have to xfail this since @return_arg_callee is treated as an opaque 
-; function, and the anlysis couldn't prove that %b and %c are not aliases 
-; XFAIL: *
-
 define i32* @return_arg_callee(i32* %arg1, i32* %arg2) {
   ret i32* %arg1
 }
 ; CHECK-LABEL: Function: test_return_arg
 ; CHECK: NoAlias: i32* %a, i32* %b
 ; CHECK: MayAlias: i32* %a, i32* %c
 ; CHECK: NoAlias: i32* %b, i32* %c
 define void @test_return_arg() {
   %a = alloca i32, align 4
   %b = alloca i32, align 4
 
   %c = call i32* @return_arg_callee(i32* %a, i32* %b)
 
   ret void
-}
+}