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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
