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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:128
       }
       // Check if this is a direct call to a known function.
       if (CalledFunction) {
----------------
The comment is outdated.



================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:134
+            SmallVector<DevirtCallSite, 1> DevirtCalls;
+            SmallVector<CallInst *, 1> Assumes;
+            bool HasNonAssumeUses;
----------------
Is 1 the common expectation?


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:137
+            findDevirtualizableCallsForTypeTest(DevirtCalls, Assumes,
+                                                HasNonAssumeUses, CI);
+            if (HasNonAssumeUses) {
----------------
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;
}
```


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:138
+                                                HasNonAssumeUses, CI);
+            if (HasNonAssumeUses) {
+              auto TypeMDVal = cast<MetadataAsValue>(CI->getArgOperand(1));
----------------
A comment about why only adding type test that only have an assume as user would be welcome for the reader here.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:145
+          // Skip all other intrinsics.
           continue;
+        }
----------------
Just a nit, you can do early continue here:

```
if (CalledFunction->getIntrinsicID() != Intrinsic::type_test)
    continue;
...
```




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



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


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5050
+    case bitc::FS_TYPE_TESTS: {
+      PendingTypeTests.insert(PendingTypeTests.end(), Record.begin(),
+                              Record.end());
----------------
Can we assert `PendingTypeTests.empty()` here?


================
Comment at: llvm/test/Bitcode/thinlto-type-tests.ll:30
+
+declare i1 @llvm.type.test(i8*, metadata) nounwind readnone
----------------
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).


https://reviews.llvm.org/D27967