[PATCH] D45673: [x86] Fix PR37100 by teaching the EFLAGS copy lowering to rewrite uses across basic blocks in the limited cases where it is very straight forward to do so.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp:445-457
+      if (&TestMBB != &UseMBB && !MDT->dominates(&TestMBB, &UseMBB)) {
+        DEBUG({
+          dbgs() << "ERROR: Encountered use that is not dominated by our test "
+                    "basic block! Rewriting this would require inserting PHI "
+                    "nodes to track the flag state across the CFG.\n\nTest "
+                    "block:\n";
+          TestMBB.dump();
----------------
mzolotukhin wrote:
> Could all of this be under `DEBUG`? The fatal error we're reporting here looks like an internal compiler assert anyway.
> If we put in entirely under `DEBUG`, we can avoid building dominators in Release compilers.
The reason we've kept a number of these as fatal errors is due to our concern about latent problems in LLVM. We're essentially introducing a pretty novel set of constraints on what kinds of EFLAGS copies can be lowered, and trying to get somewhat more reliable failure modes if this bites us...

And at least one of these already helped uncover a bug (that this patch is fixing)...

Is the domtree analysis cost showing up significantly in compile times for you? Are there other concerns?


Repository:
  rL LLVM

https://reviews.llvm.org/D45673