[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

[Evan Cheng]

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

> 
> 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?

That adds a second branch inside a loop. That can be plenty bad for small tight loops.

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

This is in PHI elimination which is explicitly asking for machineloopinfo. Can it ever be null?

Evan

> 
> /jakob
>