[PATCH] D46706: [PM/LoopUnswitch] Support partial trivial unswitching.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: llvm/include/llvm/Analysis/Utils/Local.h:144
 
+/// Delete all of the instructions in the provided vector, and all other
+/// instructions that deleting these in turn causes to be trivially dead.
----------------
Why not s/provided vector/`DeadInsts`?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:76
+///
+/// This essentially walks
+/// from a root recursively through loop variant operands which have the exact
----------------
Nit: wrap?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:322
+    ExitDirection = false;
+    // std::swap(CondVal, Replacement);
     LoopExitSuccIdx = 1;
----------------
Stray line?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:333
 
-  DEBUG(dbgs() << "    unswitching trivial branch when: " << CondVal
-               << " == " << LoopCond << "\n");
+  // For partial unswitching of a condition, we need the exit block to be
+  // reached directly from the partially unswitched input. This can be done when
----------------
Would be nice to be a bit more specific "For partial trivial unswitching of a condition".


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:398
+    IRBuilder<> IRB(OldPH);
+    Value *Cond = Invariants.front();
+    for (Value *Invariant :
----------------
Might be nice to assert here that if `LoopExitSuccIdx == 0` then `cast<Instruction>(BI.getCondition())->getOpcode()` is `Or` etc.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch.ll:511
+  %cond_or4 = or i1 %cond_or3, %cond4
+  br i1 %cond_or4, label %loop_exit, label %do_something
+; CHECK:       loop_begin:
----------------
I think we need to test a few more cases here:

 - Branch on chain of `and`s
 - Branch on mix of `and`s and `or`s and possibly other operations


Repository:
  rL LLVM

https://reviews.llvm.org/D46706