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

Filipe Cabecinhas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 2 09:49:57 PST 2017

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?


More information about the llvm-commits mailing list