[PATCH] D35840: All libcalls should be considered to be GC-leaf functions.

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In https://reviews.llvm.org/D35840#822820, @dneilson wrote:

> In https://reviews.llvm.org/D35840#822809, @anna wrote:
>
> > This change looks good to me. However, I would suggest separating out the `PlaceSafepoints` (and it's testcase) change from the `RewriteStatepointsForGC` change. We don't use placeSafepoint anymore, so we should perhaps just deprecate it upstream, instead of fixing it. 
> >  @reames: What do you suggest?
>
>
> With the codebase in its current state, the PlaceSafepoints & RewriteStatepointsForGC changes cannot be separated. Fundamentally, the change is to behaviour of callsGCLeafFunction() -- making it flag lib calls as gc-leaf. That change required changing the prototype for the function, which in turn required the change in PlaceSafepoints.
>
> The change could be contained to add the libcall check to just RewriteStatepointsForGC in the NeedsRewrite lambda, but that's less of a proper fix in my view -- it fixes a location of the bug's manifestation, but not the source of the bug.


SGTM!



================
Comment at: lib/Transforms/Utils/Local.cpp:1850
 
+  // Lib calls can be materialized by some passes, and won't be
+  // marked as 'gc-leaf-function.' All available Libcalls are
----------------
dneilson wrote:
> anna wrote:
> > Please state that libcalls can be materialized by RS4GC, there's no other passes right? (frankly, the placeSafepoints.cpp is deprecated).
> > 
> That's not true. Libcalls can be created by other passes than just RS4GC. For example, where we noticed this problem was in a series of simplifications:
> pow(x, 2) -> exp2(x) -> ldexp(x, 2)
> pow & exp2 are both intrinsics, so we'd have been fine with that simplification, but ldexp does not have an intrinsic form, so the standard lib version is used (i.e. a libcall).
ah thanks for the clarification.


https://reviews.llvm.org/D35840