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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn 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);
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1246
       InsertedValues.insert(AddRecPhiMatch);
+      InsertedInstructions.push_back(AddRecPhiMatch);
       // Remember the increment.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84327