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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:392
 
+  std::string getGlobalName(VTableSlot Slot, ArrayRef<uint64_t> Args,
+                            StringRef Name);
----------------
Document new functions


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:765
   }
+  CSInfo.TypeCheckedLoadUsers.clear();
 }
----------------
There are now a bunch of places where this array is being cleared. Suggest putting this in a helper method called something like CSInfo.markDevirtualized() - it will make it more explicit as to what is going on.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:794
+    if (CSInfo.isExported()) {
+      Res->TheKind = WholeProgramDevirtResolution::ByArg::UniqueRetVal;
+      Res->Info = IsOne;
----------------
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)? 


================
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
----------------
Why check these - is there a chance they could be modified if the optimization goes wrong?


================
Comment at: llvm/test/Transforms/WholeProgramDevirt/import.ll:40
   ; UNIFORM-RET-VAL: call i1 %
+  ; UNIQUE-RET-VAL0: call i1 %
+  ; UNIQUE-RET-VAL1: call i1 %
----------------
Add comment on why the optimization can't/shouldn't be applied here.


https://reviews.llvm.org/D29917