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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jvesely.

p.s. You know you're changing the old pass which isn't used in the new pass manager right?  You may instead wish to focus on SimpleLoopUnswitch which is the new version.



================
Comment at: include/llvm/IR/IRBuilder.h:1795
+  /// the new value.
+  Value *CreateFreezeAtDef(Value *Arg, Function *F, const Twine &Name = "",
+                           bool replaceAllUses = true) {
----------------
This is really not idiomatic for an IRBuilder function.  The standard idioms is to just insert the requested instruction sequence at the specified insertion point.  Please refactor to match that idiom and move other logic to caller or utility outside of IRBuilder.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:229
     bool UnswitchIfProfitable(Value *LoopCond, Constant *Val,
+                              bool NeedFreeze = true,
                               TerminatorInst *TI = nullptr);
----------------
Please leave this out of the current patch.  It belongs in the one using the must execute logic.

I'd be open to combining them if you thought that was needed.   


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:897
+
+  IRBuilder<> Builder(LoopPreheader);
+  if (isa<TerminatorInst>(LIC)) {
----------------
I think this can and should be simpler.  If we need a freeze, just insert it into the end of the preheader.  The complexity of hoisting to def doesn't seem warranted.  


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

https://reviews.llvm.org/D29015