[PATCH] D29808: WholeProgramDevirt: Add any unsuccessful llvm.type.checked.load devirtualizations to the list of llvm.type.test users.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1095
+    // devirtualized, we need to add the resulting llvm.type.test intrinsics to
+    // the function summaries so that the LowerTypeTests pass will export them.
+    if (Action == PassSummaryAction::Export && isa<MDString>(S.first.TypeID)) {
----------------
mehdi_amini wrote:
> pcc wrote:
> > mehdi_amini wrote:
> > > pcc wrote:
> > > > mehdi_amini wrote:
> > > > > I'm still not sure what/why this is done. What happens in which phase? 
> > > > > What is the LowerTypeTests pass exporting exactly, and for which consumer?
> > > > Essentially this is applying the same transformation to the summary as we would make to the function itself with regular LTO.
> > > > 
> > > > In the exporting phase we need to figure out how each llvm.type.checked.load intrinsic will be lowered. There are two cases:
> > > > 1. (not devirtualized) emit a regular virtual call and an llvm.type.test
> > > > 2. (devirtualized) emit a devirtualized call without an llvm.type.test
> > > > This change is necessary to correctly handle case 1. By adding the type ID to the list of type tests, we cause the exporting LowerTypeTests pass to export the summary information for that type ID (i.e. it will fill in the TypeTestResolution field in TypeIdSummary). That information will be consumed by the importing LowerTypeTests pass in the thin backend, after the importing WholeProgramDevirt pass has transformed the llvm.type.checked.load into an llvm.type.test (based on the WholeProgramDevirtResolution's Indir resolution for the type ID).
> > > > 
> > > > In case 2 the TypeCheckedLoadUsers vector will be cleared (see D29811) so this code will end up not adding the type test to the summary.
> > > I think I may just get it now, but still not totally sure: the `llvm.type.test` in case 1 is useful for CFI only right? 
> > Correct.
> OK, so I'd really like that anything that is done for CFI is clearly identified/documented as such (when in a context that deals with devirt), otherwise it makes it hard to understand what's going on without having both in mind. 
> It is absolutely non-intuitive to me when reviewing (or for a reader of) `WholeProgramDevirt.cpp` that these specific actions are performed only for CFI purpose (technically we should be able to *not* do these when CFI isn't enabled by the way).
> 
I would have thought that the references to "llvm.type.checked.load" would have made this clear enough (per the langref link mentioned earlier, the intrinsic is specific to CFI). But okay, I'll try to put something here with a summary of how llvm.type.checked.load intrinsics are handled by this pass.


https://reviews.llvm.org/D29808