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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Looks okay, just some nits.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3593
+bool InstCombinerImpl::freezeDominatedUses(FreezeInst &FI) {
+  Value* Op = dyn_cast<Value>(FI.getOperand(0));
+
----------------
nikic wrote:
> Casting to Value makes no sense, it's already a Value. (Okay, it's a Use, but it decays to Value.)
This comment still holds. In fact all three `dyn_cast`s in this function are unnecessary.


================
Comment at: llvm/test/Transforms/InstCombine/freeze.ll:140
+; If freeze is applied to function argument, replace all
+; uses of arg to freeze(arg).
+
----------------
Comment is outdated.


================
Comment at: llvm/test/Transforms/InstCombine/freeze.ll:189
+  ret void
+}
----------------
I think it would be good to add the special case of there being a second freeze of the same value, as we see in the adjusted PhaseOrdering tests. In that case we effectively end up CSEing the freezes as a side-effect of this transform.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106233