[PATCH] D29015: [LoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Is `NeedFreeze` always `true`? If so, why have the parameter?
If not, when is it not?



================
Comment at: include/llvm/IR/IRBuilder.h:1718
+    FreezeInst *FI = nullptr;
+
+    if (Instruction *I = dyn_cast<Instruction>(Arg)) {
----------------
Maybe add
```if (isa<FreezeInst>(Arg))
  return Arg;```
so we don't need to create a `FreezeInst` that will be immediately simplified in the next run of instsimplify (and avoids `ReplaceAllUsesWith`)?
Unless I missed something.


================
Comment at: include/llvm/IR/IRBuilder.h:1734
+    } else {
+      llvm_unreachable("unhandled value to freeze");
+    }
----------------
What happens if we try to `Freeze` a non-`Instruction`, non-`Argument` `Value` subclass (any type of `Constant`?)?
>From the name and location, I'm assuming this function should be fairly generic.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:243
     bool UnswitchIfProfitable(Value *LoopCond, Constant *Val,
+                              bool NeedFreeze = true,
                               TerminatorInst *TI = nullptr);
----------------
Why is it a default parameter here?


https://reviews.llvm.org/D29015