samestep added inline comments.
================ Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:348 + auto *Loc = Env.getReturnStorageLocation(); + assert(Loc != nullptr); + // FIXME: Model NRVO. ---------------- sgatev wrote: > samestep wrote: > > sgatev wrote: > > > Let's do `if (Loc == nullptr) return;` > > I don't think we want to do that, right? Shouldn't the `return` storage > > location always be set? Or is this about the "analyzing fragments rather > > than full functions" thing we discussed yesterday? > I think it's related. If we are going with always initializing the `return` > storage location then I guess at some point we should be able to make > `Environment::getReturnStorageLocation` return a reference? In that case I'm > fine with keeping the assert around in the meantime. OK; yeah, I think the intention is that we're always initializing it. I'll leave this code as is for now, then. ================ Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:564 + assert(ReturnLoc != nullptr); + Env.setStorageLocation(*S, *ReturnLoc); + Env.popCall(ExitEnv); ---------------- sgatev wrote: > samestep wrote: > > sgatev wrote: > > > We use stable storage locations to ensure convergence. In that spirit, > > > shouldn't we assign `ReturnLoc`'s value to `S`'s storage location instead > > > of changing the storage location? Alternatively, we can pass `S`'s > > > storage location to `pushCall` so that it can store it as `ReturnLoc`. > > Could you clarify how this hurts convergence? My understanding is that > > `ReturnLoc` here is already stable, so this would make `S`'s storage > > location stable too. > If I follow correctly, `ReturnLoc` here is the result of > `Env.createStorageLocation(ReturnType)` which isn't stable. Each call to > `createStorageLocation` returns a fresh storage location. Ah I see, you're right. Is there a way to make a stable storage location for the `return`? My intuition is that we can't just pass `S`'s storage location to `pushCall`, because we want the storage location for the `return` to be the same across analysis of different callsites to the callee (similar to how we currently use the same storage location for a given parameter of the callee, regardless of how many times we analyze it). But maybe it would be fine; @ymandel do you have any thoughts on this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130600/new/ https://reviews.llvm.org/D130600 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
