[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added a comment.

This patch is to be rebased against D73094 <https://reviews.llvm.org/D73094>



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:830
   enum Kind {
+    Unknown,   ///< Unknown (analysis not performed, don't lower)
     Unsat,     ///< Unsatisfiable type (i.e. no global has this type metadata)
----------------
I don't think such implicit conversion of Unsat to Unknown is good idea. I guess problem will arise for legacy index files: for those ByteArray resolution will become Unsat and so on. I suggest moving Unknown the end of the list and bumping index version respectively (parseTypeIdSummaryRecord doesn't verify TheKind).


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1380
+  // in ICP (which is performed earlier than this in the regular LTO pipeline).
+  MPM.addPass(LowerTypeTestsPass(nullptr, nullptr, true));
 
----------------
Can we get rid of two identical passes here (and in other places also)? For instance 
```
MPM.addPass(LowerTypeTestsPass(ExportSummary, nullptr, true));
```
can both lower type metadata and do final cleanup.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+    // breaks any uses on assumes.
+    if (TypeIdMap.count(TypeId))
+      continue;
----------------
I don't think, I understand this.
It looks like that if (a) we have type.test in the module and (b) we don't have vtable definition with corresponding type metadata in the same module then we'll remove type.test/assume sequence before even getting to ICP. This seems strange in the context of previous discussion because virtual function may be called in different module from one where vtable is defined, like so:
```
struct A { virtual int foo() { return 42; } };

// calls pA->foo
extern int callFoo(A *pA);
int main() { A a; return callFoo(&a); }
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73242/new/

https://reviews.llvm.org/D73242