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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

In D80908#2069984 <https://reviews.llvm.org/D80908#2069984>, @eugenis wrote:

> Missing roundtrip textual and binary IR encoding test. And I think it won't pass because you are reading getUpper and writing getSignedMax (to textual) and those seem off-by-one.


it's thinlto-function-summary-paramaccess.ll:18 and it works.
Asm read/write uses getSignedMax. I think for [A,B] it's more intuitive to read this as inclusive range
binary read/write uses getUpper



================
Comment at: llvm/include/llvm/Bitcode/LLVMBitCodes.h:297
+  // [n x (paramno, range, numcalls, numcalls x (callee_guid, paramno, range))]
+  FS_STACK_SAFETY_PARAMS = 25,
 };
----------------
eugenis wrote:
> Should we call it something like ParamAccessRange here and everywhere? W/o stack safety in the name.
> 
if you don't mind FS_PARAM_ACCESS as range just a current type, e.g in future it can be something more different.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80908