[PATCH] D106233: [InstCombine] Add freezeAllUsesOfArgument to visitFreeze
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 19 11:50:57 PDT 2021
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
More information about the llvm-commits
mailing list