[PATCH] D106233: [InstCombine] Add freezeAllUsesOfArgument to visitFreeze

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3593
+bool InstCombinerImpl::freezeDominatedUses(FreezeInst &FI) {
+  Value* Op = dyn_cast<Value>(FI.getOperand(0));
+
----------------
Casting to Value makes no sense, it's already a Value. (Okay, it's a Use, but it decays to Value.)


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3598
+
+  SmallVector<Use *> UsesToBeUpdated;
+  for (auto &U : Op->uses())
----------------
Maybe use `replaceUsesWithIf()`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3601
+    if (dyn_cast<Instruction>(U.getUser()) &&
+        DT.dominates(dyn_cast<Value>(&FI), dyn_cast<Instruction>(U.getUser())))
+      UsesToBeUpdated.push_back(&U);
----------------
You should be checking domination on `Use`, not the user. For example, a use in a phi might be dominated even though the phi is not dominated.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3621
+  if (freezeDominatedUses(I))
+    return &I;
+
----------------
I think you should do this after the other optimizations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106233