[llvm-bugs] [Bug 37332] New: ARC optimizations interact badly with catchswitch

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=37332

            Bug ID: 37332
           Summary: ARC optimizations interact badly with catchswitch
           Product: libraries
           Version: trunk
          Hardware: PC
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: Scalar Optimizations
          Assignee: unassignedbugs at nondot.org
          Reporter: smeenai at fb.com
                CC: ahatanak at gmail.com, compnerd at compnerd.org,
                    llvm-bugs at lists.llvm.org, rjmccall at apple.com,
                    rnk at google.com

Created attachment 20260
  --> https://bugs.llvm.org/attachment.cgi?id=20260&action=edit
Reduced IR

If you run the attached IR with opt -objc-arc, the verifier will complain
because ARC optimizations attempted to insert an objc_release before a
catchswitch, but a catchswitch must be the only non-PHI instruction in its
basic block. Specifically, the ARC optimizations were attempting to duplicate
an objc_release across both edges of an invoke (presumably to shorten a
lifetime), so they wanted an insertion point that was guaranteed to be reached
from the unwind edge of the invoke; unfortunately, with the funclet-style
exception instructions, there's no such guaranteed insertion point in the
existing CFG in the general case.

One way I see of fixing this is splitting the invoke edge if it goes to a
catchswitch and inserting a cleanuppad to hold the release. E.g. in my
particular example the transformed IR would be:

  %call = invoke i8* @f(i8* %p, i8* %q)
            to label %invoke.cont unwind label %ehcleanup

ehcleanup:
  %tmp = cleanuppad within none []
  tail call void @objc_release(i8* %p) [ "funclet"(token %tmp) ]
  cleanupret from %tmp to label %catch.dispatch

%catch.dispatch:
  ...

I think this is the "right" thing to do in some sense, but it also involves
changing the CFG, which is probably going to be fraught with pain. I admittedly
haven't looked too much into how much work it would be to keep the analysis
correct in the face of a CFG change, but I'm guessing there's a reason the
existing transform explicitly avoids splitting edges :) (See
https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp;331493$262-272.)

The other option is to just bail out of the objc_release movement if there's a
catchswitch in the way. Unfortunately, I can't figure out where to do that
check. Marking SetCFGHazardAfflicted inside
BottomUpPtrState::HandlePotentialUse doesn't seem to do the trick; I think the
CFGHazardAfflicted state somehow gets lost at some point during merging. I can
check for the presence of a catchswitch inside ObjCARCOpt::CheckForCFGHazards,
but that seems overly conservative, since we only care if the catchswitch is a
potential insertion point. Any help on that front would be very appreciated.

(I'm also not sure why the retain/release for %p isn't just elided entirely in
this particular example. I have a separate reduction for that, and I'll file a
different bug.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20180504/3fe3c47f/attachment.html>