[PATCH] D27582: Avoid infinite loops in branch folding

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: lib/CodeGen/BranchFolding.cpp:1637
+          PrevBB.isSuccessor(&*FallThrough) &&
+          !FallThrough->isEHPad()) {
         MBB->moveAfter(&MF.back());
----------------
rnk wrote:
> Let's check isEHPad before analyzeBranch. It's cheaper, and it makes no sense to try to arrange a fallthrough to an EH pad. The analogy with landingpads would be this situation:
> ```
>   invoke ... to %unreachable unwind %lpad
> next: ; cannot arrange fallthrough for some reason
>   ret void
> lpad:
>   landingpad ...
> ```
> Without your change, it looks like we'll try to get lpad to follow the invoke, which isn't useful.
OK, that definitely makes sense.


================
Comment at: test/CodeGen/X86/branchfolding-catchpads.ll:98
+
+define void @test3() optsize personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
----------------
rnk wrote:
> Can we manually make this simpler?
You'd really think so, wouldn't you?  I tried removing most of these pieces and even taking out one of the arguments to the g() call or removing one of the switch branches to unreachable blocks avoids the infinite loop.  I strongly suspect that this combination of things is just pushing it over some threshold for a size optimization, but I have to admit that I don't completely understand it.  I'm running the test without optimizations (llc does default to OptNone, right?), but even just removing the optsize function attribute causes this test case not to hang.  I'll try a run that dumps before every pass to see if I can figure out what's really going on.

Honestly, I'm not sure how useful this is as a regression test.  It reproduces the failure right now, but I have no reason to think that it will still reproduce the failure a month from now.  What it does accomplish, however, is documenting the kind of complexity that can trigger this bug.  When I reverted the previous fix, I did so because I had convinced myself that this failure wasn't possible anymore after trying a few variations on the other two test cases in this file.  I'd like to hope that anyone looking at this test case in the future would just think "OK, let's just assume that it's still possible."  Maybe that's better done by adding some foreboding comments in the code.


Repository:
  rL LLVM

https://reviews.llvm.org/D27582