This is an archive of the discontinued LLVM Phabricator instance.

Improve RefreshCallGraph to remove invalid call graph edge.
ClosedPublic

Authored by hulx2000 on Feb 17 2015, 3:12 PM.

Details

Reviewers
chandlerc
apazos
Summary

This patch is to improve CGPassManager::RefreshCallGraph to remove an invalid call graph edge corrupted by Instruction Combiner, triggered an assert in CallGraphNode::addCalledFunction later.

With commit d8214db0868f228026fa6ab1d5ca6c76ab3b5ce0, llvm's Instruction Combiner now can turn an sqrtl into a llvm.fabs.f64, the call graph edge (to be more precise, one Entry in B's CalledFunctions list) originally representing call to sqrtl now becomes invalid: the instruction is the call instruction for llvm.fabs.f64 (done by Value:: replaceAllUsesWith(..)), and the call graph node is still sqrtl, note that intrinsic shouldn't be in call graph, and call graph node for sqrtl should be gone too.

The problem can only be reproduced if we meet the all following conditions:

  1. Compile the program with NDK sysroot, and program need to be complicate enough to invoke CGPassManager::RunAllPassesOnSCC.
  2. For program with function A and B, A calls B, B calls some other functions, B need to be compiled before A to reproduce the problem.
  3. While compiling function B, inlining is invoked and instruction combiner is able to turn a call to sqrtl into llvm.fabs.f64
  4. Inlining is invoked while compiling function A (a bigger inlining threshold is needed)

This problem can't be reproduced with llvm trunk, because it doesn't turn a sqrtl into llvm.fabs.f64, but I think it is a real problem.

Diff Detail

Repository
rL LLVM

Event Timeline

hulx2000 retitled this revision from to Improve RefreshCallGraph to remove invalid call graph edge..
hulx2000 updated this object.
hulx2000 edited the test plan for this revision. (Show Details)
hulx2000 added reviewers: apazos, chandlerc.
hulx2000 set the repository for this revision to rL LLVM.
hulx2000 added a subscriber: Unknown Object (MLST).
hulx2000 edited the test plan for this revision. (Show Details)Feb 25 2015, 10:06 AM
apazos edited edge metadata.Mar 9 2015, 1:15 PM

I would like to commit this bug fix today.

Chandler and other reviewers, are you ok with this change?

http://reviews.llvm.org/D7705

Ana.

chandlerc accepted this revision.Apr 14 2015, 6:20 AM
chandlerc edited edge metadata.

Sorry for the delay reviewing this. LGTM, and yea, I can imagine it would be hard to craft a test case.

This revision is now accepted and ready to land.Apr 14 2015, 6:20 AM
hulx2000 closed this revision.Jul 9 2015, 4:52 PM

chad merge it under commit 2fbccf6972c1468e3d252d2d3125403c4065ffff

sorry, didn't notice the status change until now.