[llvm-commits] patch: CXAGuardElimination pass.

[Eli Friedman]

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.

> 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