[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)) {
----------------
it's going to make instruction safe even if user is not alloca

For consistency isSafeMemIntrinsicAccess will help


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:260
+                    const SCEV *AccessSize);
+
 public:
----------------
fmayer wrote:
> vitalybuka wrote:
> > Can you replace typeSizeToSCEV with two overloads?
> > 
> > ```
> > bool isSafeAccess(... Value *AccessSize)
> > 
> > bool isSafeAccess(...TypeSize AccessSize)
> > ```
> I think that just adds more complexity, as this way we will need 
> 
> bool isSafeAccess(... Value *AccessSize)
> bool isSafeAccess(... TypeSize AccessSize)
> 
> which only do the conversion and call some
> 
> isSafeAccess(... SCEV *AccessSize).
> 
> Doing the conversion explicitly in the caller seems more readable to me.
please do this to remove SCEV stuff from that long loop


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:367-369
+  const SCEV *Min = toCharPtr(SE.getConstant(Size.getLower()));
+  const SCEV *Max = SE.getMinusSCEV(toCharPtr(SE.getConstant(Size.getUpper())),
+                                    toCharPtr(AccessSize));
----------------
vitalybuka wrote:
> In all 3 cases can you please replace 
> toCharPtr with getIntNTy, at least for readability not sure if it make a difference for SCEV
maybe: can you make toCharPtr same local lambda? or you plan to use it outside?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113160