[llvm-commits] [llvm] r140083 - /llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp

[Bill Wendling]

On Sep 20, 2011, at 12:32 AM, Duncan Sands wrote:

> Hi Bill,
> 
>> If we are extracting a basic block that ends in an invoke call, we must also
>> extract the landing pad block. Otherwise, there will be a situation where the
>> invoke's unwind edge lands on a non-landing pad.
>> 
>> We also forbid the user from extracting the landing pad block by itself. Again,
>> this is not a valid transformation.
>> 
>> Modified:
>>     llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
>> 
>> Modified: llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp?rev=140083&r1=140082&r2=140083&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp Mon Sep 19 18:00:52 2011
>> @@ -664,7 +664,13 @@
>>    //  * Pass in uses as args
>>    // 3) Move code region, add call instr to func
>>    //
>> -  BlocksToExtract.insert(code.begin(), code.end());
>> +  for (std::vector<BasicBlock*>::const_iterator
>> +         I = code.begin(), E = code.end(); I != E; ++I) {
>> +    BasicBlock *BB = *I;
>> +    BlocksToExtract.insert(BB);
>> +    if (InvokeInst *II = dyn_cast<InvokeInst>(BB->getTerminator()))
>> +      BlocksToExtract.insert(II->getUnwindDest());
>> +  }
> 
> I think the design of ExtractCodeRegion is that the caller should already have
> taken care of this kind of thing.  For example, take a look at the following
> lines (// Assumption: this is a single-entry code region, ...).

Well, it's still single-entry. ;-) I thought about that when I did this, and I could go either way on it. It *does* seem nicer to have the CodeExtractor be simplistic, but it would make calling ExtractBasicBlock more complicated. Basically, the caller would have to make sure that the landing pad was included when necessary and that its critical edge is split...

Six to one... Half dozen to the other...

>>    Values inputs, outputs;
>> 
>> @@ -788,6 +794,7 @@
>>  /// ExtractBasicBlock - slurp a basic block into a brand new function
>>  ///
>>  Function* llvm::ExtractBasicBlock(BasicBlock *BB, bool AggregateArgs) {
>> +  if (BB->isLandingPad()) return 0;
> 
> Wouldn't it be better to put this logic in CodeExtractor::isEligible?
> 
Okay, sure.

-bw