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

Michael Zolotukhin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 19 00:43:23 PDT 2018


mzolotukhin 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();
----------------
chandlerc wrote:
> 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?
I see a value of having it on at least for some time, but I think in the long run we should turn it into an assert. Would it be possible?

I just noticed a new pass in the pipeline and wanted to check if it can be avoided. On O0 dom-tree analysis costs ~0.25% geomean (on CTMark), so each separate invocation doesn't seem to cost much, but they add up eventually.

By the way, why didn't we catch the bug you are talking about with RA compiler bots? Maybe that's an argument for adding more RA bots?


Repository:
  rL LLVM

https://reviews.llvm.org/D45673





More information about the llvm-commits mailing list