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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson added a comment.

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.



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


https://reviews.llvm.org/D35840