[PATCH] D91711: SCEV add function to see if SCEVUnknown is null

Wed Nov 25 02:02:20 PST 2020

markus added a comment.

In D91711#2415349 <https://reviews.llvm.org/D91711#2415349>, @mkazantsev wrote:

> The idea of having SCEV pointing to deleted instructions scares me. Imagine what if we had SCEVAddRecs referencing the loops we've already deleted.  There is a vast field for nasty bugs caused by UB and memory corruption. I'd rather expect that we fail some assertion if we try to optimize with SCEV in that state.

I agree. Clearly such a SCEV would not be valid and should no longer be used for any purpose at all. However by calling `checkValidity()` a client could guard against such situations if the client knows that part of the IR may have been deleted during transformation. For the existing SCEV clients (except for the LSR debug salvaging use I did and mentioned in the llvm-dev post) they already know that IR is still around, and nothing needs to be done.

To me it seems that functionality is already in place and it is mostly a matter of clarification (and if favorable making `checkValidity()` public).

I think there are meaningful use cases such as https://reviews.llvm.org/D87494 but of course if a decision is taken in the opposite direction then that patch needs to be reverted. Either way is fine with me as long as it is well-founded.

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12469
+  });
+}
+
----------------
nikic wrote:
> This method is the same as `checkValidity()`. I can't really comment on whether exposing it publicly makes sense or not.
That is very interesting, I had not noticed that before. I see that `checkValidity()` is only used by `getExistingSCEV()` which appears to be some kind of caching mechanism.

Clearly one do not want to return a cached SCEV result if the expression has a SCEVUknown pointing at some Value that has been deleted.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91711