[PATCH] D113160: [stack-safety] Check SCEV constraints at memory instructions.
Vitaly Buka via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 19 18:52:57 PST 2021
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
More information about the llvm-commits
mailing list