[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:260
+                    const SCEV *AccessSize);
+
 public:
----------------
Can you replace typeSizeToSCEV with two overloads?

```
bool isSafeAccess(... Value *AccessSize)

bool isSafeAccess(...TypeSize AccessSize)
```


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:319-325
   if (const auto *MTI = dyn_cast<MemTransferInst>(MI)) {
     if (MTI->getRawSource() != U && MTI->getRawDest() != U)
       return ConstantRange::getEmpty(PointerSize);
   } else {
     if (MI->getRawDest() != U)
       return ConstantRange::getEmpty(PointerSize);
   }
----------------
why don't we do this in case if isSafeAccess?


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:353
+
+bool StackSafetyLocalAnalysis::isSafeAccess(const Use &UI, AllocaInst *AI,
+                                            const Instruction *I,
----------------
UI -> U


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:354
+bool StackSafetyLocalAnalysis::isSafeAccess(const Use &UI, AllocaInst *AI,
+                                            const Instruction *I,
+                                            const SCEV *AccessSize) {
----------------
I is not needed, it can be retrived from U


================
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));
----------------
In all 3 cases can you please replace 
toCharPtr with getIntNTy, at least for readability not sure if it make a difference for SCEV


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:399
         if (AI && !SL.isAliveAfter(AI, I)) {
-          US.addRange(I, UnknownRange);
+          US.addRange(I, UnknownRange, /*IsSafe=*/false);
           break;
----------------
maybe add for this case a dedicated function US.addUnsaveAccess(I);


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:466
+          auto TypeSize = DL.getTypeStoreSize(CB.getParamByValType(ArgNo));
+          auto AccessRange = getAccessRange(UI, Ptr, TypeSize);
+          US.addRange(I, AccessRange,
----------------
for consistency 
either (i prefer this one):
```
          US.addRange(I, getAccessRange(UI, Ptr, TypeSize);,
                                  isSafeAccess(UI, AI, I, typeSizeToSCEV(TypeSize)));
```
or:

```
auto AccessRange = getAccessRange(UI, Ptr, TypeSize);
bool Safe = isSafeAccess(UI, AI, I, typeSizeToSCEV(TypeSize));
US.addRange(I, AccessRange, Safe);
```


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:343
+    return SE.getCouldNotCompute();
+  auto *PtrTy = IntegerType::getInt8PtrTy(SE.getContext());
+  return SE.getConstant(PtrTy, TS.getFixedSize());
----------------
eugenis wrote:
> It's a little strange to compute type size in a pointer, but SCEV does not care as long as the number of bits is the same, so I guess it is fine.
> 
```
auto *IntTy = IntegerType::getIntNTy(SE.getContext(), PointerSize);
  return SE.getConstant(IntTy, TS.getFixedSize());

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113160