[PATCH] D124526: [SimpleLoopUnswitch] Collect either logical ANDs/ORs but not both.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2766
     while (match(Cond, m_Select(m_Value(CondNext), m_One(), m_Zero())))
       Cond = CondNext;
     BI->setCondition(Cond);
----------------
nikic wrote:
> Looks like this code tries to handle this special case as well.

Yeah, the code here already handles `select _, true, false` by skipping it and updating the branch condition before reaching `collectHomogenousInstGraphLoopInvariants`. But this is not ideal, as it can change the function even if we do not perform unswitching and claim we didn't change the function.

I will update the patch with an alternative approach, that replaces all instances where we use `m_LogicalAnd`/`m_LogicalOr` with helpers that skip `select _, true, false`.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2780
       TinyPtrVector<Value *> Invariants =
-          collectHomogenousInstGraphLoopInvariants(L, CondI, LI);
+          collectHomogenousInstGraphLoopInvariants(L, CondI, None, LI);
       if (Invariants.empty())
----------------
nikic wrote:
> Can we also determine ExitDirection here? I think the implementation of           collectHomogenousInstGraphLoopInvariants() would be a good deal cleaner if we always passed in whether we expect or/and, rather than doing a guess in some of the cases.
Unfortunately I don't think we can. I think here both target blocks may be in the loop and the unswitching direction is determined based on the match operation tree.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124526