[PATCH] D80908: [StackSafety] Add info into function summary

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka marked 12 inline comments as done.
vitalybuka added a comment.

Thanks for feedback. Docs are here https://reviews.llvm.org/D82941
I'll create a new patch for unique_ptr<> change



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:556
 
+  static constexpr uint32_t StackSafetyRangeWidth = 64;
+  /// Describes the use of a value in a call instruction, specifying the call's
----------------
tejohnson wrote:
> vitalybuka wrote:
> > eugenis wrote:
> > > This can be safely reduced to 32 bits or even less in practice, if that would help with the binary size.
> > Writer/Parser push them as uint64 and smart encoded store them efficiently
> > on StackSafety side i plan to cut-off large values, e.g. 2^32 but it's going to be a separate patch
> If these will be max 32 bits can you use that here? Even though they will be compressed on serialization, I assume it would affect in-memory size.
If you don't mind I'll fix "max 32" later, with other algorithm optimizations.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:625
+  /// Uses for every parameter to this function.
+  std::vector<ParamAccess> ParamAccesses;
+
----------------
tejohnson wrote:
> Since this will only be used under stack safety (which I assume is optional and uncommon?), might this be better as a unique_ptr like we do for type id info declared just above? The overhead of an empty vector is going to be higher (3x on my system), which will add up in the combined summary.
I'll create a separate patch for this tomorrow.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:6286
+    case bitc::FS_PARAM_ACCESS: {
+      PendingParamAccesses = parseParamAccesses(Record);
+      break;
----------------
tejohnson wrote:
> Will the use of this summary info be able to correctly distinguish from an old bitcode file (created before this support was added) from one created after that simply doesn't have this summary info for some reason? Does it need to for correct operation?
Missing info is not a problem. Users of StackSafety just will not be able to optimize out safety checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80908