[PATCH] D80399: [Local] Prevent `invertCondition` from creating a redundant instruction

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

I support this enhancement. Thanks for doing it!



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:3067-3068
-    if (isa<PHINode>(Inst)) {
-      // FIXME: This fails if the inversion is to be used in a
-      // subsequent PHINode in the same basic block.
-      Inverted->insertBefore(&*Parent->getFirstInsertionPt());
----------------
I don't think the current change fixes this issue. As far as I can see, it can't even be fixed inside this function since the intended use is not visible here. It might help to move the comment to the beginning of the function so that it is more obvious to users.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:3072
+  auto *Inverted =
+      BinaryOperator::CreateNot(Condition, Condition->getName() + ".inv");
+  if (Inst && !isa<PHINode>(Inst))
----------------
This name change affects a lot of FileCheck directives. I was there once, and backed off very very slowly. :D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80399