[llvm] r177680 - Always forward 'resume' instructions to the outter landing pad.

[Duncan Sands]

Hi Bill,

On 22/03/13 19:54, Bill Wendling wrote:
> On Mar 22, 2013, at 1:44 AM, Duncan Sands <baldrick at free.fr> wrote:
>
>> Hi Bill,
>>
>> On 22/03/13 00:30, Bill Wendling wrote:
>>> Author: void
>>> Date: Thu Mar 21 18:30:12 2013
>>> New Revision: 177680
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=177680&view=rev
>>> Log:
>>> Always forward 'resume' instructions to the outter landing pad.
>>>
>>> How did this ever work?
>>>
>>> Basically, if you have a function that's inlined into the caller, it may not
>>> have any 'call' instructions, but any 'resume' instructions it may have should
>>> still be forwarded to the outer (caller's) landing pad. This requires that all
>>> of the 'landingpad' instructions in the callee have their clauses merged with
>>> the caller's outer 'landingpad' instruction (hence the bit of ugly code in the
>>> `forwardResume' method).
>>
>> does this fix PR14116 too?  There is a comprehensive test case in comment 10.
>>
> I have no clue. :) I'll have to check.

OK :)

>>> +  // Merge the catch clauses from the outer landing pad instruction into the
>>> +  // inlined landing pad instructions.
>>> +  for (SmallPtrSet<LandingPadInst*, 16>::iterator I = InlinedLPads.begin(),
>>> +         E = InlinedLPads.end(); I != E; ++I) {
>>> +    LandingPadInst *InlinedLPad = *I;
>>> +    for (unsigned OuterIdx = 0, OuterNum = OuterLPad->getNumClauses();
>>> +         OuterIdx != OuterNum; ++OuterIdx) {
>>> +      bool hasClause = false;
>>> +      if (OuterLPad->isFilter(OuterIdx)) continue;
>>
>> Likewise.  Also, is this actually correct for C++, since the filter may not be
>> filtering everything out, only some things.
>>
> I don't know what you mean here. I'm skipping the 'filter' clause because it's not relevant to adding catch clauses here...

But you have to add filters too, otherwise it may not be correct!  The entire
list of things the exception is compared against has to be appended, i.e. all
catch clauses but also all filters.  Of course you can often then simplify
things, but the logical operation is appending everything from the outer
landing pad onto the inner one.  If you look at the instcombine logic it knows
how to simplify filters, and is careful to do it correctly; you can't always
just throw them away.

>
>>> +      Value *OuterClause = OuterLPad->getClause(OuterIdx);
>>> +      for (unsigned Idx = 0, N = InlinedLPad->getNumClauses(); Idx != N; ++Idx)
>>> +        if (OuterClause == InlinedLPad->getClause(Idx)) {
>>> +          hasClause = true;
>>> +          break;
>>> +        }
>>> +      if (!hasClause)
>>> +        InlinedLPad->addClause(OuterClause);
>>> +    }
>>> +  }
>>>   }
>>
>> I'm kind of confused as to what the above logic is doing.  Why don't you just
>> append all of the outer clauses onto the inner landing pad, always, without
>> any special thinking?
>>
> Because I don't want multiple clauses that's catching the same value. It clutters up the IR and can cause the EH tables to become very large.

Instcombine will clean it all up...  It knows all this and more.  I think that
doing some simplification in the inliner would be advantageous if it allowed you
to avoid inlining some basic blocks.  But I don't think it ever does: it is
basic blocks in the function being inlined into that can become unreachable, and
I don't think you have a good way of detecting and dropping those in the
inliner, but I could be wrong.  Is instcombine failing to clean stuff up?

Ciao, Duncan