evgeny777 added inline comments.
================
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));
----------------
tejohnson wrote:
> 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?
> 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?
Ok, I see. I don't think having this another option is significantly better. So
let's leave it as is for now
================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+ // breaks any uses on assumes.
+ if (TypeIdMap.count(TypeId))
+ continue;
----------------
tejohnson wrote:
> 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.
> 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 was thinking of slightly different approach: it seems you have required type
information in combined index together with type name in the module, so you
probably can add external declarations of required vtables (this may require
promotion) to the module in the ICP pass and run ICP over them. Do you think
this can work?
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
