[PATCH] [CallGraph] Given -print-callgraph a stable printing order.

[Justin Bogner]

Sanjoy Das <sanjoy at playingwithpointers.com> writes:
> Hi bogner, chandlerc,
>
> Since FunctionMap maps Function* (pointers) the order in which the
> traversal happens can differ from run to run, causing spurious FileCheck
> failures.  Have CallGraph::print sort the CallGraphNodes by name before
> printing them.

LGTM once my comments below are addressed.

> http://reviews.llvm.org/D10575
>
> Files:
>   lib/Analysis/IPA/CallGraph.cpp
>   test/Analysis/CallGraph/non-leaf-intrinsics.ll
>
> Index: lib/Analysis/IPA/CallGraph.cpp
> ===================================================================
> --- lib/Analysis/IPA/CallGraph.cpp
> +++ lib/Analysis/IPA/CallGraph.cpp
> @@ -98,8 +98,29 @@
>      OS << "<<null function: 0x" << Root << ">>\n";
>    }
>  
> -  for (CallGraph::const_iterator I = begin(), E = end(); I != E; ++I)
> -    I->second->print(OS);
> +  // To be able to conveniently check -print-callgraph's output using FileCheck,
> +  // we sort the CallGraphNodes by name before printing them.  Another way to
> +  // achieve this is to make FunctionMap compare the keys by name and not
> +  // pointer value.  However, that would slow down the normal, no-printing case
> +  // in favor of speeding this exceptional case.

I'm not sure that the discussion about FileCheck is really adding
anything, so it's probably sufficient to say:

  // Print in a deterministic order by sorting CallGraphNodes by name.
  // We do this here to avoid slowing down the non-printing fast path.

> +
> +  SmallVector<CallGraphNode *, 16> Nodes;
> +  Nodes.reserve(FunctionMap.size());
> +
> +  for (auto I = begin(), E = end(); I != E; ++I)
> +    Nodes.push_back(I->second);
> +
> +  std::sort(Nodes.begin(), Nodes.end(),
> +            [](CallGraphNode *LHS, CallGraphNode *RHS) {
> +    if (Function *LF = LHS->getFunction())
> +      if (Function *RF = RHS->getFunction())
> +        return LF->getName() < RF->getName();
> +
> +    return LHS->getFunction() == nullptr;

I think you want "return RHS->getFunction() != nullptr" here so that we
get a strict weak ordering. That is, if both LHS and RHS are null we
need to return false for this to satisfy the requirements of std::sort.

> +  });
> +
> +  for (CallGraphNode *CN : Nodes)
> +    CN->print(OS);
>  }
>  
>  #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
> Index: test/Analysis/CallGraph/non-leaf-intrinsics.ll
> ===================================================================
> --- test/Analysis/CallGraph/non-leaf-intrinsics.ll
> +++ test/Analysis/CallGraph/non-leaf-intrinsics.ll
> @@ -25,8 +25,8 @@
>  ; CHECK: Call graph node <<null function>>
>  ; CHECK:  CS<0x0> calls function 'f'
>  
> -; CHECK: Call graph node for function: 'calls_statepoint'
> -; CHECK-NEXT:  CS<[[addr_0:[^>]+]]> calls external node
> -
>  ; CHECK: Call graph node for function: 'calls_patchpoint'
>  ; CHECK-NEXT:  CS<[[addr_1:[^>]+]]> calls external node
> +
> +; CHECK: Call graph node for function: 'calls_statepoint'
> +; CHECK-NEXT:  CS<[[addr_0:[^>]+]]> calls external node