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

[Florian Mayer via Phabricator via llvm-commits]

fmayer 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)) {
----------------
vitalybuka wrote:
> 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?
> 
> 
Good point, if `!AI` in `isSafeAccess` we should also return true, for consistency. Then the semantics remain well-defined and the same: "all *stack* accesses done by this instruction are safe.

The callers check `findAllocaForValue` to make sure this is definitely a stack access before calling `stackAccessIsSafe`.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:121
   ConstantRange Range;
-  std::map<const Instruction *, ConstantRange> Accesses;
+  std::set<const Instruction *> UnsafeAccesses;
 
----------------
vitalybuka wrote:
> 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
Can we do this in a separate change? This doesn't really belong here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113160