[PATCH] D27967: IR: Function summary representation for type tests.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:134
+            SmallVector<DevirtCallSite, 1> DevirtCalls;
+            SmallVector<CallInst *, 1> Assumes;
+            bool HasNonAssumeUses;
----------------
mehdi_amini wrote:
> Is 1 the common expectation?
Yes, basically the pattern created by the frontend is: `@llvm.assert(@llvm.type.test(...))`. I think it could only be different if GVN or some other pass managed to combine two calls to `@llvm.type.test`. I've only seen that happen once or twice in chromium though.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:137
+            findDevirtualizableCallsForTypeTest(DevirtCalls, Assumes,
+                                                HasNonAssumeUses, CI);
+            if (HasNonAssumeUses) {
----------------
mehdi_amini wrote:
> So we don't care about `DevirtCalls` and about `Assumes` but we're still gonna populate them?
> 
> Why extending this API and calling it when it seems that what you want is simply:
> 
> ```
> bool HasNonAssumeUses = llvm::any_of(CI->uses(), [] (const Use &CIU) {
>     return !isa<CallInst>(CIU.getUser());
> }
> ```
> 
> Even though it should likely rather be:
> 
> ```
> bool HasNonAssumeUses = llvm::any_of(CI->uses(), [] (const Use &CIU) {
>     if (auto *AssumeCI = dyn_cast<CallInst>(CIU.getUser())) {
>       Function *F = AssumeCI->getCalledFunction();
>       if (F && F->getIntrinsicID() == Intrinsic::assume) 
>         return false;
>     }
>     return true;
> }
> ```
Works for me, I was getting a little ahead of myself to when we'll need to look at the calls for devirtualization. But that can all happen later.


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:75
     auto AssumeCI = dyn_cast<CallInst>(CIU.getUser());
     if (AssumeCI) {
       Function *F = AssumeCI->getCalledFunction();
----------------
mehdi_amini wrote:
> Unrelated but looks like it could be `if (auto *AssumeCI = dyn_cast<CallInst>(CIU.getUser()))`
> 
r290264


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:78
       if (F && F->getIntrinsicID() == Intrinsic::assume)
         Assumes.push_back(AssumeCI);
+    } else {
----------------
mehdi_amini wrote:
> Technically, the else path here should also set `HasNonAssumeUses = true`
I ended up reverting this part.


================
Comment at: llvm/test/Bitcode/thinlto-type-tests.ll:30
+
+declare i1 @llvm.type.test(i8*, metadata) nounwind readnone
----------------
mehdi_amini wrote:
> Can you add a test with a type.test without any user, and another with a type.test that only has an assume as user?
> (Since you special case these).
llvm/test/Bitcode/thinlto-unused-type-tests.ll


https://reviews.llvm.org/D27967