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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.
Herald added a reviewer: jdoerfert.

I completely missed this patch (my filter didn't pick it up since it doesn't mention LTO). I have some post-commit comments on it.

Is there another patch somewhere that shows how this info is used?



================
Comment at: llvm/docs/LangRef.rst:6828
+where the first ``param`` is the number of the parameter it describes,
+``offset`` is the known access range of the paramenter inside of the function.
+
----------------
Logically, what is "offset"? is this the byte offset from the pointer? What does it mean if this info doesn't exist - I assume that means unknown and therefore a need to be conservative?


================
Comment at: llvm/docs/LangRef.rst:6839
+the number of the callee parameter which points into the callers parameter
+with offset known to be inside of the ``offset`` range.
+
----------------
Can you show a small example? I assume the offset of each Callee in the list must be a subset of the offset of the containing Param?


================
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
----------------
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.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:556
 
+  /// Describes the uses of a parameter by the range of offsets accessed in the
+  /// function and all of the call targets it is passed to.
----------------
Byte offsets I assume?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:567
+      GlobalValue::GUID Callee = 0;
+      ConstantRange Offsets{RangeWidth, true};
+
----------------
Document constant parameter (here and in declaration for Use below)


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:625
+  /// Uses for every parameter to this function.
+  std::vector<ParamAccess> ParamAccesses;
+
----------------
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.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:672
+StackSafetyInfo::getParamAccesses() const {
+  assert(needsParamAccessSummary(*F->getParent()));
+
----------------
I realize this is only for debug compilers, but it seems like an expensive assert - it walks all functions in the module, and we will call this function/assert once per function.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:684
+    Param.Calls.reserve(PS.Calls.size());
+    for (auto &C : PS.Calls) {
+      if (C.Offset.isFullSet()) {
----------------
Please add comments to this code to describe logically what/why it is doing. I.e. why does it not add any param info if any offset "isFullSet".


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:6286
+    case bitc::FS_PARAM_ACCESS: {
+      PendingParamAccesses = parseParamAccesses(Record);
+      break;
----------------
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?


================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:38
 
+constexpr uint32_t FunctionSummary::ParamAccess::RangeWidth;
+
----------------
Is this needed? I don't see a use here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80908