[PATCH] D113160: [stack-safety] Check SCEV constraints at memory instructions.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:452
+          auto AccessRange = getMemIntrinsicAccessRange(MI, UI, Ptr);
+          bool Safe = false;
+          if (const auto *MTI = dyn_cast<MemTransferInst>(MI)) {
----------------
fmayer wrote:
> vitalybuka wrote:
> > it's going to make instruction safe even if user is not alloca
> > 
> > For consistency isSafeMemIntrinsicAccess will help
> This is consistent with the API this is used in
> 
> ```
>   // Returns true if the instruction can be proven to do only two types of
>   // memory accesses:
>   //  (1) live stack locations in-bounds or
>   //  (2) non-stack locations.
> ```
> 
> If the user is not alloca, that is (2).
>> it's going to make instruction safe even if *user* is not alloca
*user* -> location

isSafeAccess() line 355 
return false for !AI

I don't like to have undefined behavior for (2) even if you don't use it now. It's trivial to make it defined either true or false. However if instruction access function argument, we don't know if it a stack of another function. So unsafe (false) seems reasonable choose here. BTW why this is not important for current callers?




================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:121
   ConstantRange Range;
-  std::map<const Instruction *, ConstantRange> Accesses;
+  std::set<const Instruction *> UnsafeAccesses;
 
----------------
fmayer wrote:
> kstoimenov wrote:
> > I am not sure what is the ratio of unsafe to safe accesses is, but I would expect that more of them are unsafe, right? In that case it makes more sense to have a set of SafeAccesses to use less memory. 
> That doesn't work though. What we want is 
> 
> any(instr is unsafe for alloca for all allocas reachable)
> 
> for that we need to keep track of the unsafe instructions for each alloca.
Just noticed:
Why this is in UseInfo and per alloca, and then we merge them latter anyway?
It should be FunctionInfo::UnsafeAccesses?

Then you pass that reference to the set as new arg of analyzeAllUses


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113160