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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Sorry for being late to the party, but a couple of optional post commit style comments for possible follow up.  Nothing major, just ideas on how to share code and reduce a possible ordering sensitivity.



================
Comment at: llvm/trunk/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:81
+static SmallVector<Value *, 4>
+collectHomogenousInstGraphLoopInvariants(Loop &L, Instruction &Root,
+                                         LoopInfo &LI) {
----------------
Hm, with some slight generalization, this could be a generally useful utility elsewhere.  This is basically handling reduction operations spelled with multiple instructions.  Seems like that's a common enough pattern (say, instcombine?) to be worth drawing out into a generic visitReductionOperands(Root, FilterFunc)?


================
Comment at: llvm/trunk/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:248
+
+  if (L.isLoopInvariant(BI.getCondition())) {
+    Invariants.push_back(BI.getCondition());
----------------
I believe you've got an pass ordering sensitivity here which isn't present in the old pass.  The old pass deliberately uses "makeLoopInvariant" which is like isLoopInvariant, but it will hoist trivially hoistable instructions as well.  By not doing this, the new pass will be very sensitive to having ideal (i.e. fully LICMed) IR.  This may be problematic if any other loop pass is run between LICM and the new unswitch.  


================
Comment at: llvm/trunk/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:349
+             "Must have an `and` of `i1`s for the condition!");
+    for (Value *Invariant :
+         make_range(std::next(Invariants.begin()), Invariants.end()))
----------------
Hm, possibly out of scope, but I'll mention it anyway.

This ends up producing a reduce chain rather than a tree.  We're going to end up trying to reassociate that later.  Is it worthwhile having an interface which produces an appropriate tree to start with?  Maybe this is an API that make sense on IR Builder?  Something like CreateOr(ArrayRef<Value> Ops)?


================
Comment at: llvm/trunk/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:377
+  // be entered.
+  ConstantInt *Replacement = ExitDirection
+                                 ? ConstantInt::getFalse(BI.getContext())
----------------
Hm, we have this pattern all over.  Might be time for a getBoolean(bool) on ConstantInt?


Repository:
  rL LLVM

https://reviews.llvm.org/D46706