[StackSafety,NFC] Update documentation

[vitalybuka]

Summary

It's follow up for D80908

[vitalybuka]

format

[vitalybuka]

fixes

[tejohnson]

Thanks, a few follow on questions and nits below.

llvm/docs/LangRef.rst

6875

nit: s/Then/The/

I assume if it is passed directly to another call and we don't know the accesses within that callee (this is the per-module summary) that it would have a calls entry with offset [0,0]?

6878

nit: "increments or decrements the parameter pointer and passes it.."

llvm/include/llvm/IR/ModuleSummaryIndex.h

557

nit: s/in in/in/

The "by all of the call targets it is passed to" sounds ambiguous to me, as if we already have knowledge of how it is accessed within those callees. I guess this is the situation after the thin link, but not initially.

Probably best to be more explicit. I.e., from my understanding reading your LangRef writeup:

• In the per-module summary, the Calls vector summarize the byte offset applied to each pointer parameter before passing to each corresponding callee. I.e. this structure describes offsets computed from the pointer parameter within the function only
• In the combined summary, the offsets may include the offsets within each called function consuming this pointer parameter. Two questions: I assume that happens after some kind of inter-procedural propagation across the combined summary? After that propagation, is the Calls list empty?

[vitalybuka]

update

[vitalybuka]

llvm/docs/LangRef.rst

6875

I assume if it is passed directly to another call and we don't know the accesses within that callee (this is the per-module summary) that it would have a calls entry with offset [0,0]?

calls list is only needed when we don't know accesses inside of callee. If we know accesses we can directly apply them to offset of parameter (offset: [5, 5])
if we don't know accesses we still know offset used to pass the parameter
I've added function body into the example.

llvm/include/llvm/IR/ModuleSummaryIndex.h

557

I moved this description close to fields.

[vitalybuka]

update

[tejohnson]

LGTM, thanks!

llvm/docs/LangRef.rst

 where each ``Param`` describes pointer parameter access inside of the
 function and looks like:
 
 .. code-block:: text
 
     param: 4, offset: [0, 5][, calls: ((Callee)[, (Callee)]*)]?
 
 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.
+``offset`` is the inclusive range of offsets from the pointer parameter to bytes
+which can be accessed by the function. This range does not include accesses by
+function calls from ``calls`` list.
 
 where each ``Callee`` decribes how parameter is forwared into other
 functions and looks like:
 
 .. code-block:: text
 
     callee: ^3, param: 5, offset: [-3, 3]
 
 The ``callee`` refers to the summary entry id of the callee,  ``param`` is
 the number of the callee parameter which points into the callers parameter
-with offset known to be inside of the ``offset`` range.
+with offset known to be inside of the ``offset`` range. ``calls`` will be
+consumed and removed by thin link stage to update ``Param::offset`` so it
+covers all accesses possible by ``calls``.
+
+Pointer parameter without corresponding ``Param`` is considered unsafe and we
+assume that access with any offset is possible.
+
+Example:
+
+If we have the following function:
+
+.. code-block:: text
+
+    define i64 @foo(i64* %0, i32* %1, i8* %2, i8 %3) {
+      store i32* %1, i32** @x
+      %5 = getelementptr inbounds i8, i8* %2, i64 5
+      %6 = load i8, i8* %5
+      %7 = getelementptr inbounds i8, i8* %2, i8 %3
+      tail call void @bar(i8 %3, i8* %7)
+      %8 = load i64, i64* %0
+      ret i64 %8
+    }
+
+We can expect the record like this:
+
+.. code-block:: text
+
+    params: ((param: 0, offset: [0, 7]),(param: 2, offset: [5, 5], calls: ((callee: ^3, param: 1, offset: [-128, 127]))))
+
+The function may access just 8 bytes of the paramenter %0 . ``calls`` is empty,
+so the parameter is either not used for function calls or ``offset`` already
+covers all accesses from nested function calls.
+Parameter %1 escapes, so access is unknown.
+The function itself can access just a single byte of the parameter %2. Additional
+access is possible inside of the ``@bar`` or ``^3``. The function adds signed
+offset to the pointer and passes the result as the argument %1 into ``^3``.
+This record itself does not tell us how ``^3`` will access the parameter.
+Parameter %3 is not a pointer.
 
 .. _refs_summary:
 
 Refs
 ^^^^
 
 The optional ``Refs`` field looks like:
 

llvm/include/llvm/Analysis/StackSafetyAnalysis.h

public:
   ~StackSafetyInfo();
 
   const InfoTy &getInfo() const;
 
   // TODO: Add useful for client methods.
   void print(raw_ostream &O) const;
 
   /// Parameters use for a FunctionSummary.
+  /// Function collects access information of all pointer parameters.
+  /// Information includes a range of direct access of parameters by the
+  /// functions and all call sites accepting the parameter.
+  /// StackSafety assumes that missing parameter information means possibility
+  /// of access to the parameter with any offset, so we can correctly link
+  /// code without StackSafety information, e.g. non-ThinLTO.
   std::vector<FunctionSummary::ParamAccess> getParamAccesses() const;
 };
 
 class StackSafetyGlobalInfo {
 public:
   struct InfoTy;
 
 private:

llvm/include/llvm/IR/ModuleSummaryIndex.h

struct FFlags {
     unsigned ReturnDoesNotAlias : 1;
 
     // Indicate if the global value cannot be inlined.
     unsigned NoInline : 1;
     // Indicate if function should be always inlined.
     unsigned AlwaysInline : 1;
   };
 
-  /// 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.
+  /// Describes the uses of a parameter by the function.
   struct ParamAccess {
     static constexpr uint32_t RangeWidth = 64;
 
     /// Describes the use of a value in a call instruction, specifying the
     /// call's target, the value's parameter number, and the possible range of
     /// offsets from the beginning of the value that are passed.
     struct Call {
       uint64_t ParamNo = 0;
       GlobalValue::GUID Callee = 0;
-      ConstantRange Offsets{RangeWidth, true};
+      ConstantRange Offsets{/*BitWidth=*/RangeWidth, /*isFullSet=*/true};
 
       Call() = default;
       Call(uint64_t ParamNo, GlobalValue::GUID Callee,
            const ConstantRange &Offsets)
           : ParamNo(ParamNo), Callee(Callee), Offsets(Offsets) {}
     };
 
     uint64_t ParamNo = 0;
-    ConstantRange Use{RangeWidth, true};
+    /// The range contains byte offsets from the parameter pointer which
+    /// accessed by the function. In the per-module summary, it only includes
+    /// accesses made by the function instructions. In the combined summary, it
+    /// also includes accesses by nested function calls.
+    ConstantRange Use{/*BitWidth=*/RangeWidth, /*isFullSet=*/true};
+    /// In the per-module summary, it summarizes the byte offset applied to each
+    /// pointer parameter before passing to each corresponding callee.
+    /// In the combined summary, it's empty and information is propagated by
+    /// inter-procedural analysis and applied to the Use field.
     std::vector<Call> Calls;
 
     ParamAccess() = default;
     ParamAccess(uint64_t ParamNo, const ConstantRange &Use)
         : ParamNo(ParamNo), Use(Use) {}
   };
 
   /// Create an empty FunctionSummary (with specified call edges).

llvm/lib/Analysis/StackSafetyAnalysis.cpp

for (auto &FnKV : Info->Info) {
       }
     }
     if (StackSafetyPrint)
       print(errs());
   }
   return *Info;
 }
 
-// Converts a StackSafetyFunctionInfo to the relevant FunctionSummary
-// constructor fields
 std::vector<FunctionSummary::ParamAccess>
 StackSafetyInfo::getParamAccesses() const {
+  // Implementation transforms internal representation of parameter information
+  // into FunctionSummary format.
   std::vector<FunctionSummary::ParamAccess> ParamAccesses;
   for (const auto &KV : getInfo().Info.Params) {
     auto &PS = KV.second;
+    // Parameter accessed by any or unknown offset, represented as FullSet by
+    // StackSafety, is handled as the parameter for which we have no
+    // StackSafety info at all. So drop it to reduce summary size.
     if (PS.Range.isFullSet())
       continue;
 
     ParamAccesses.emplace_back(KV.first, PS.Range);
     FunctionSummary::ParamAccess &Param = ParamAccesses.back();
 
     Param.Calls.reserve(PS.Calls.size());
     for (auto &C : PS.Calls) {
+      // Parameter forwarded into another function by any or unknown offset
+      // will make ParamAccess::Range as FullSet anyway. So we can drop the
+      // entire parameter like we did above.
+      // TODO(vitalybuka): Return already filtered parameters from getInfo().
       if (C.Offset.isFullSet()) {
         ParamAccesses.pop_back();
         break;
       }
       Param.Calls.emplace_back(C.ParamNo, C.Callee->getGUID(), C.Offset);
     }
   }
   return ParamAccesses;