[PATCH] D139876: [LoopSimplify] Restoring the normalization of br instructions.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.

In D139876#3990029 <https://reviews.llvm.org/D139876#3990029>, @nikic wrote:

> This is almost certainly not the correct way to fix whatever problem you're trying to fix. If an optimization doesn't work with commuted branch targets, then that optimization needs to be fixed.

There is always a tradeoff between having a canonical form and expecting this canonical form in transforms, and trying to support everything. I've seen enough cases where exit by false isn't [fully] supported, so saying that "fallthrough stays in loop is our canonical form" isn't a that bad idea in general.

Besides, even if it's not about the optimizations, codegen will still do something similar when it tries to lay out the blocks. So generally I like the idea of this patch (though I think the place for doing it is wrong).



================
Comment at: llvm/lib/Transforms/Utils/LoopSimplify.cpp:543
+          if (isa<BinaryOperator>(BI->getCondition())) {
+            if (dyn_cast<BinaryOperator>(BI->getCondition())->swapOperands())
+              BI->swapSuccessors();
----------------
How is that even possible?
UB here.


================
Comment at: llvm/lib/Transforms/Utils/LoopSimplify.cpp:545
+              BI->swapSuccessors();
+          } else if (isa<ICmpInst>(BI->getCondition())) {
+            dyn_cast<ICmpInst>(BI->getCondition())->swapOperands();
----------------
`m_match(BI, m_Br(m_ICmp (...) )`


================
Comment at: llvm/lib/Transforms/Utils/LoopSimplify.cpp:548
+            BI->swapSuccessors();
+          }
+        }
----------------
`Changed = true` somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139876