[PATCH] D68782: [ThinLTO] Import virtual method with single implementation in hybrid mode

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:999
+  if (ValueInfo TheFnVI = ExportSummary->getValueInfo(TheFn->getGUID()))
+    AddCalls(SlotInfo, TheFnVI);
 
----------------
It might be good to add a comment here about why the return value can be ignored (any needed promotion would have been taken care of when the LTO unit was split IIRC).


================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:1861
           for (Metadata *MD : MetadataByGUID[VF.GUID]) {
-            CallSlots[{MD, VF.Offset}]
-                .CSInfo.markSummaryHasTypeTestAssumeUsers();
+            CallSlots[{MD, VF.Offset}].CSInfo.addSummaryTypeTestAssumeUser(FS);
           }
----------------
Probably should go ahead and fold markSummaryHasTypeTestAssumeUsers into addSummaryTypeTestAssumeUser since that is the only callsite now.


================
Comment at: test/ThinLTO/X86/devirt_single_hybrid.ll:24
+
+; IMPORT:       define available_externally hidden i32 @_ZNK1A1fEv(%struct.A* %this)
+; IMPORT-NEXT:  entry:
----------------
importing it certainly signals that it must have been devirtualized, but it would probably be good to test this explicitly, e.g either checking the call or the pass remark


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

https://reviews.llvm.org/D68782