[llvm-commits] [llvm] r111221 - in /llvm/trunk: lib/CodeGen/MachineBasicBlock.cpp lib/CodeGen/PHIElimination.cpp test/CodeGen/ARM/code-placement.ll test/CodeGen/X86/lsr-reuse.ll

[Jakob Stoklund Olesen]

On Aug 17, 2010, at 10:49 AM, Evan Cheng wrote:

> 
> On Aug 17, 2010, at 12:11 AM, Jakob Stoklund Olesen wrote:

>> Careful you don't dereference a NULL pointer. You can't count on MLI being available here.
> 
> Right.
> 
>> 
>> Also, I think the decision to not split a back edge should go in PHIElimination as a policy decision. Other callers of SplitCriticalEdge may have really good reasons for splitting a back edge. That should be allowed.
> 
> I have mixed feeling about this. In theory code placement optimization passes should be able to fix this. That's why the check in codegenprep's critical edge splitting is called a hack. I thought it a good idea to put the hack in one place only. Anyway, I'm moving the check to phi elimination (and fixed it) and I'll keep an eye on other clients.

Thanks.

I like the idea that SplitCriticalEdge only returns false when it would be impossible to split the edge. We could add a ShouldSplitCriticalEdge to indicate that it would be a really bad idea.

If the new block is placed after its predecessor, you get a loop with a conditional branch in the middle and an unconditional branch at the bottom. That's not so bad, is it?


@@ -392,8 +393,14 @@
      // We break edges when registers are live out from the predecessor block
      // (not considering PHI nodes). If the register is live in to this block
      // anyway, we would gain nothing from splitting.
-      if (!LV.isLiveIn(Reg, MBB) && LV.isLiveOut(Reg, *PreMBB))
-        Changed |= PreMBB->SplitCriticalEdge(&MBB, this) != 0;
+      // Avoid splitting backedges of loops. It would introduce small
+      // out-of-line blocks into the loop which is very bad for code placement.
+      if (PreMBB != &MBB &&
+          !LV.isLiveIn(Reg, MBB) && LV.isLiveOut(Reg, *PreMBB)) {
+        if (!(MLI->getLoopFor(PreMBB) == MLI->getLoopFor(&MBB) &&
+              MLI->isLoopHeader(&MBB)))
+          Changed |= PreMBB->SplitCriticalEdge(&MBB, this) != 0;
+      }
    }
  }
  return true;

You're doing it again ;-) You shouldn't expect MLI to be non-NULL here.

/jakob