[llvm-commits] [llvm] r158392 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/branch-fold.ll

[Manman Ren]

Hi Nuno,

Thanks for reviewing this.

Manman

On Jun 13, 2012, at 6:51 AM, Nuno Lopes wrote:

> Hi,
> 
>> Author: mren
>> Date: Wed Jun 13 00:43:29 2012
>> New Revision: 158392
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=158392&view=rev
>> Log:
>> SimplifyCFG: fold unconditional branch to its predecessor if profitable.
>> 
>> This patch extends FoldBranchToCommonDest to fold unconditional branches.
>> For unconditional branches, we fold them if it is easy to update the phi nodes
>> in the common successors.
>> 
>> rdar://10554090
> 
> So, if I understand correctly the code, the profitability heuristic is that if all instructions in a basic block are also computed in it's immediate predecessor, then the unconditional branch is folded.
> I have two comments:
> - When not optimizing for size, I think it makes sense to look at the branch probability metadata and be more aggressive in the folding for highly probably-taken branches. The performance impact for the not taken branch will be negligible (since the branch won't be taken very often).
This patch handles folding of unconditional branch, conditional branch was handled even without this patch.
Conditional branches are folded to its predecessor if the PHI values entering the common destination are the same.
But I will see whether we can improve folding of conditional branches.
> - Not really related with your patch, but rather a more broaden question: Now that the register allocator can rematerialize instructions when the live intervals are too long (Jakob: please correct if I'm wrong), doesn't it make sense to actually reuse all computations done in dominating basic blocks? This patch is looking for opportunities to reuse redundant computations in adjacent blocks, but I believe we could do it for arbitrary BBs, as long as one of them dominates the other.
> 
For this testing case, the earlier pass of "jump threading" created opportunities for CSE, but we don't want to add another pass of CSE. Here, we do a limited scope of CSE.
> 
> 
>> +/// checkCSEInPredecessor - Return true if the given instruction is available
>> +/// in its predecessor block. If yes, the instruction will be removed.
>> +///
>> +bool checkCSEInPredecessor(Instruction *Inst, BasicBlock *PB) {
>> +  if (!isa<BinaryOperator>(Inst) && !isa<CmpInst>(Inst))
>> +    return false;
> 
> Here you can use Inst->mayHaveSideEffects(), since all instructions without side-effects are safe to be removed (and not just binary operators and compare instructions).
> 
I will verify this and submit another patch soon :)
> Nuno