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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked 3 inline comments as done.
tejohnson added inline comments.


================
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)
----------------
evgeny777 wrote:
> 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).
Sounds like a good idea, will do.


================
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));
 
----------------
evgeny777 wrote:
> 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.
The problem is that currently under DropTypeTests=true, LTT will drop the type tests up front and return without doing any other lowering. Which is what we want in the already existing callers of LTT with DropTypeTests=true. Here, we want LTT to perform its usual lowering, and only afterwards do the dropping. We want to do this only during regular LTO, but unfortunately we can't just key off of the ExportSummary (e.g. drop at the start if ExportSummary is nullptr, and drop at the end if ExportSummary is non-nullptr), since in some cases here ExportSummary may be nullptr (i.e. regular LTO invoked via the old LTO API). So I would need to introduce another option to essentially say "drop them after lowering". I can certainly do this if you think it would be better (I was thinking about that before, but thought it might be more confusing). WDYT?


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+    // breaks any uses on assumes.
+    if (TypeIdMap.count(TypeId))
+      continue;
----------------
evgeny777 wrote:
> 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); }
> ```
> 
We will only be able to do ICP this way if we have the target vtable in the module (similar to how we currently can only do ICP if the target function is in the module). I anticipate doing something similar for vtable defs as to what we do for virtual functions, where a fake call edge is added to the summary based on the value profile results to ensure they are imported before the LTO backend ICP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242