[PATCH] D70430: CodeGenPrepare: Clear maps containing AssertingVH's before deleting instructions (PR43269)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

All the globals maps in CodeGenPrepare are sort of a mess; we don't have any clear invariants for what a CGP transform is, and is not, allowed to do.  And at least some of them contain stale pointers in practice; I'm surprised nobody has hit a crash of some sort.  I'm afraid that even if we take this, it's a ticking time bomb until someone finds another issue.

I'm not sure what the right fix is.  We could switch the maps to use weak/callback value handles, and try to handle that.  But it's not clear what we're actually supposed to do at the point the callback happens, and it's not clear the callbacks cover all the relevant changes a transform could make, again because there aren't any invariants.

I think there are a few things we should do here:

1. Addressing mode and integer promotion handling should happen in a separate loop from the other CGP transforms, so we can enforce reasonable invariants on the transforms that execute while the maps are active.
2. The addressing mode and integer promotion handling should not call RecursivelyDeleteTriviallyDeadInstructions while we're iterating over the function.  If we're going to do that sort of cleanup, we should either delay it until after we clear the maps, or hand-roll something that updates all the relevant maps correctly.

----

In terms of whether it makes sense to take this for 9.0.1, it's hard to reason about the impact on the generated code.  I'd be more comfortable with a patch that modified when/how we erase the instructions in question.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:5113
+    // deleted below; see PR43269. Clearing them is okay: since we return
+    // true below, the optimization will be re-run.
+    LargeOffsetGEPMap.clear();
----------------
The optimization will be re-run, yes, but it might not produce the same result.  We might end up performing a partial transform based on GEPs visited after this transform.


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

https://reviews.llvm.org/D70430