[PATCH] D29917: WholeProgramDevirt: Implement export/import support for unique ret val opt.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:794
+    if (CSInfo.isExported()) {
+      Res->TheKind = WholeProgramDevirtResolution::ByArg::UniqueRetVal;
+      Res->Info = IsOne;
----------------
tejohnson wrote:
> I was trying to prove to myself that Res != nullptr when isExported(), but I am having trouble doing so. isExported() will be true when the CSInfo has SummaryTypeCheckedLoadUsers or SummaryHasTypeTestAssumeUsers - these are set up when (Action == PassSummaryAction::Export). But Res will only be non-null when (Action == PassSummaryAction::Export && isa<MDString>(S.first.TypeID)).
> 
> Is there a case where we could reach here when Action == PassSummaryAction::Export but !isa<MDString>(S.first.TypeID)? 
I think isExported() can only possibly be true for type IDs of type MDString because we only add MDStrings to MetadataByGUID (see http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/WholeProgramDevirt.cpp#1204) and we only ever set SummaryTypeCheckedLoadUsers or SummaryHasTypeTestAssumeUsers on members of MetadataByGUID.


================
Comment at: llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll:33
+
+; CHECK: @vt3a = constant i1 (i8*, i32, i32)* @vf3a
+ at vt3a = constant i1 (i8*, i32, i32)* @vf3a, !type !0
----------------
tejohnson wrote:
> Why check these - is there a chance they could be modified if the optimization goes wrong?
Yes, as with D29846 I want to make sure we aren't taking the general VCP code path.


================
Comment at: llvm/test/Transforms/WholeProgramDevirt/import.ll:40
   ; UNIFORM-RET-VAL: call i1 %
+  ; UNIQUE-RET-VAL0: call i1 %
+  ; UNIQUE-RET-VAL1: call i1 %
----------------
tejohnson wrote:
> Add comment on why the optimization can't/shouldn't be applied here.
I refreshed this change past r296948 which added a comment at the top of this function.


https://reviews.llvm.org/D29917