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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D46706#1138515, @reames wrote:

> 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.


No problem. Mostly following up here on the more discussion-y points.



================
Comment at: llvm/trunk/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:248
+
+  if (L.isLoopInvariant(BI.getCondition())) {
+    Invariants.push_back(BI.getCondition());
----------------
reames wrote:
> 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.  
Yes, this has been true throughout the new pass. IMO, it makes the pass quite a bit simpler as we get the invariant that if we didn't unswitch something, we didn't mutate the IR. And that matters a lot more in the new PM with caching of analyses.

I'd rather fix the pass pipelines to be much more careful about running LICM in the appropriate places. So far we haven't hit any issues, but it is definitely something that may come up in the future.

Does that seem reasonable?


================
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()))
----------------
reames wrote:
> 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)?
I think we canonicalize to chains pretty commonly to make analysis easier anyways, and rely on later passes forming trees where beneficial (most places).


Repository:
  rL LLVM

https://reviews.llvm.org/D46706