[llvm-commits] patch: CXAGuardElimination pass.

[Nick Lewycky]

Eli Friedman wrote:
> On Sun, May 24, 2009 at 11:25 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>> Eli Friedman wrote:
>>> On Sun, May 24, 2009 at 9:34 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>>>> However, it doesn't simplify it down all the way. See llvm.org/PR4261 for an
>>>> example of what happens after this optimization is applied on the above
>>>> program. We may decide that PR4261 is too hard to fix in general and just
>>>> add some extra logic to this pass, but I'd rather have this committed for a
>>>> start.
>>> Couldn't you just change AcquireRet from a constant 1 to a constant 0?
>>>  If it's safe to remove the guard, I don't see how the chosen path
>>> could make a difference.
>> Release has to run. It has the visible effect of changing the guard
>> variable to a 1. Nowhere in this pass do we prove those two calls are
>> the only places looking at the guard variable.
> 
> Assuming it's actually a guard, nothing besides the guard should care
> whether the guard variable initialized or not in the current call. The
> pass as written already makes assumptions that agree with that: for
> example, it doesn't bother to check for instructions with side-effects
> after the call to __cxa_guard_release.
> 
>> Visible to who? Well, I'm not sure. There is a "__cxa_guard_abort" that
>> could be used. I'm not sure what C++ input you would need to create in
>> order to make it depend on this and be broken by the change, but I'm
>> opting for the relatively safe transform.
> 
> __cxa_guard_abort only gets triggered if the constructor throws an
> exception, which shouldn't be possible here.

Okay then! I'm not going to bother reattaching a new patch, the only 
change is this new implementation:

void CXAGuardElimination::removeGuardPair(CallInst *A, CallInst *R) {
   A->replaceAllUsesWith(AcquireRet);
   A->eraseFromParent();
   R->eraseFromParent();
}

It still replaces the call to acquire with 1 but because there's no 
store on the release the whole function does in fact melt away.

Which brings me to my next point: this patch needs some testing. This 
transform doesn't trigger on llvm-test but does pass an llvm-gcc 
bootstrap. I'd appreciate it if people who have code that uses this 
would try it out.

Nick

>> ALSO, AcquireRet is used twice, once when we've decided to delete the
>> guards but also earlier when following the flow from acquire() to
>> release(). If we used the wrong value there we would flow directly into
>> the "ret void" even if there were interesting code being guarded. (Note
>> that currently "ret void" means that we didn't find the release() call
>> and therefore wouldn't eliminate it anyways.)
> 
> Ah, oops, I wasn't suggesting to change that use.
> 
> -Eli
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>