[PATCH] D84327: [SCEVExpander] Add helper to clean up instrs inserted while expanding.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1119
   if (!SI->isAtomic() && !LI->isAtomic())
     NewCall = Builder.CreateMemCpy(StoreBasePtr, SI->getAlign(), LoadBasePtr,
                                    LI->getAlign(), NumBytes);
----------------
fhahn wrote:
> jroelofs wrote:
> > I think the interface between SCEVExpanderCleaner and SCEVExpander should be a subscriber/consumer model, and the cleaner should own the tracking of what gets added, rather than the expander. Then SCEVExpanderCleaner can hook into IRBuilderCallbackInserter to track these too.
> I am not sure. What would the benefit be? SCEVExpander already needs to track which values it inserted for some internal optimizations, so hooking up 2 different callbacks seems to make things more complex than they need to be. But as mentioned inline earlier, we don't really need the extra vector, we can combine the sets and sort them.
Oh, never mind. I misread the early return on line 1127 as post-dominating this insertion, and thought it needed to be cleaned up if that happened.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1246
       InsertedValues.insert(AddRecPhiMatch);
+      InsertedInstructions.push_back(AddRecPhiMatch);
       // Remember the increment.
----------------
fhahn wrote:
> jroelofs wrote:
> > Where does `InsertedValues` differ from `InsertedInstructions`? Should the former be re-used with an appropriate `isa<Instruction>()` filter, rather than keeping two separate lists?
> The main purpose of a separate list is to keep track of the insertion order, while InsertedValues/InsertedPostIncValues are sets. We could trade the extra memory cost for extra compile-time cost, by combing the sets and sort them by. dominance when it comes to deleting.
Ah, ok. Sounds not worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84327