[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:260
+                    const SCEV *AccessSize);
+
 public:
----------------
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.


================
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);
   }
----------------
vitalybuka wrote:
> why don't we do this in case if isSafeAccess?
Added this and a test that fails without this. Thanks!


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

Although this is called `UI` in `analyzeAllUses`.


================
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;
----------------
vitalybuka wrote:
> maybe add for this case a dedicated function US.addUnsaveAccess(I);
Let's hold off of this for now, as that needs some changes (UnknownRange is in StackSafetyLocalAnalysis).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113160