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

I'm going to change my mind on this one. That's not safe. Here's my example.

We have a function that calls __cxa_guard_acquire/release on a single 
guard variable. Then we decide to clone it as part of partial 
specialization.

The initialization being guarded depends on some global variable -- if 
it's zero it does nothing, else it prints "foo". When cloneA is run we 
know that the global is always 0 so the initialization does nothing and 
we eliminate the guards. In cloneB the global is always non-zero, so it 
always prints "foo" to the screen and we keep the guards.

When cloneA runs it needs to mark the guard variable as being "already 
initialized" or else when cloneB runs it will go ahead and print "foo" 
and it's not supposed to. You've changed the behaviour of the program.

I'm going to put removeGuardPair back the way it was before.

Nick

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