[PATCH] D110390: [SCEV] Establish control over disposition caches

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Max,

Just to check my understanding, the problem this is trying to fix is specific to a non-canonical stale SCEV right?  That is, one which is still allocated (e.g. in the bump pointer allocator), but no longer used for new expressions (e.g. has been removed from the UniqueSCEV map)?

Your example creates a stale SCEV via a RAUW operation on an instruction which contributes one of the (SCEV) operands.  I think it's important to highlight that *all* of the SCEVs for the original expression remain in the stale state.  We remove them from the bi-directional maps, but we do *not* deallocate them.

As an aside, we also create stale SCEVs quite explicitly when working with RAUW operations on SCEVUnknowns.

Though, hm, maybe we actually have two levels of staleness here.  We have "not in bi-directional maps", and we have "not in unique scev map".  Your case is really only the former, SCEV unknown is the later.  Not sure if that matters yet, or not.  (It does, see later.)

If we're on the same page with that, read further.  If not, stop here.

I think I'd lean towards validating most stale SCEVs in the disposition cache.  This necessitates considering a stale entry valid, but as you point out, we'd otherwise have to walk SCEV users which we don't have a mechanism to do.  (We seem to keep stumbling into that from different angles, maybe we should just change that?)

With that starting point, the question then becomes why the stale SCEV doesn't produce the same result?  I think the answer probably comes down to an interaction between how an interaction between SCEVUnknown::deleted() and the block disposition handling for an unknown.  If my skim of the code is correct, your verification probably crashes on "dyn_cast<Instruction>(cast<SCEVUnknown>(S)->getValue())" in computeBlockDisposition right?

I think I'm leaning towards something along the lines of "verify a stale SCEVs disposition unless it contains a SCEVUnknown which has been deleted".

Another approach would be to only verify one step of the block disposition.  E.g. "assuming operands are correctly cached, what would our block disposition be?  Is that the cached result?"  This would side step the problem by bailing out if the operand wasn't cached.  The existing forgetValue would eliminate the SCEV unknown node, and thus verification would side step the same class of stale expressions.

Neither of these are pretty, but I think they'd work and be less invasive/expensive.


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

https://reviews.llvm.org/D110390