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

Vitaly Buka via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 23 12:08:33 PST 2021


vitalybuka accepted this revision.
vitalybuka added inline comments.
This revision is now accepted and ready to land.


================
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:
> > 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`.
i don't like this assumption on what user will do
this seems like a goal of this analysis and it should tell without external tricks.

However It should be easier to do after "UnsafeAccesses" followup patch.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:121
   ConstantRange Range;
-  std::map<const Instruction *, ConstantRange> Accesses;
+  std::set<const Instruction *> UnsafeAccesses;
 
----------------
fmayer wrote:
> 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.
sure
in would be nice in follow up patches also to switch from UnsafeAccesses to SaveAccessesOnAlloca
probably we can track two sets:
a. all instructions on any alloca
b. unsafe instructions on any alloca
and then keep (a-b) for result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113160



More information about the llvm-commits mailing list