[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

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

Thank you so much for pointing that out!

[sanjoy]

lib/Analysis/CFLAliasAnalysis.cpp

371

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

371

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!

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->isDeclaration() || !Fn->hasExactDefinition();
   }
 
   bool tryInterproceduralAnalysis(CallSite CS,
                                   const SmallVectorImpl<Function *> &Fns) {
     assert(Fns.size() > 0);
 
     if (CS.arg_size() > MaxSupportedArgsInSummary)
       return false;