[PATCH] D73423: [LV] Do not try to sink dead instructions.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7163
+  // ingredients whose recipe we'll need to record. Dead instructions do not
+  // need sinking and we remove them from SinkAfter.
+  for (auto &Entry : make_early_inc_range(SinkAfter)) {
----------------
gilr wrote:
> How about cleaning sink-after once, immediately after calling collectTriviallyDeadInstrucions()?
Done, I've moved it to the caller.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7165
+  for (auto &Entry : make_early_inc_range(SinkAfter)) {
+    if (DeadInstructions.find(Entry.first) == DeadInstructions.end()) {
+      RecipeBuilder.recordRecipeOf(Entry.first);
----------------
gilr wrote:
> DeadInstructions.count()?
I am never sure which version is really preferable. I think find() makes it a bit more explicit. But I can change it to count if that's preferred, as there can be no duplicate keys in dense maps


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7199
   // control flow is preserved, we should keep them.
   auto &ConditionalAssumes = Legal->getConditionalAssumes();
   DeadInstructions.insert(ConditionalAssumes.begin(), ConditionalAssumes.end());
----------------
gilr wrote:
> Independently, this belongs in collectTriviallyDeadInstructions(), right?
I am not sure if it makes sense to put them there. As indicated in the comment, we really should handle them properly. They are not really dead, it's more that dropping them is the easiest way to handle them legally and adding them to DeadInstructions is the quickest way to not add them to the plan.

It should be initialized in the caller though I think. I'll put that up as a follow-up change.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7231
       if (isa<BranchInst>(Instr) ||
           DeadInstructions.find(Instr) != DeadInstructions.end())
         continue;
----------------
gilr wrote:
> DeadInstructions.count()?
I can update it, if preferred.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73423