[PATCH] D20593: ValueMaterializer: fuse materializeDeclFor and materializeInitFor (NFC)

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-May-24, at 12:49, Mehdi AMINI <mehdi.amini at apple.com> wrote:
> 
> joker.eph created this revision.
> joker.eph added reviewers: dexonsmith, rafael.
> joker.eph added a subscriber: llvm-commits.
> 
> They were originally separated to handle the co-recursion between
> the ValueMapper and the ValueMaterializer. This recursion does not
> exist anymore: the ValueMapper now uses a Worklist and the
> ValueMaterializer is scheduling job on the Worklist.
> 
> http://reviews.llvm.org/D20593
> 
> Files:
>  include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
>  include/llvm/Transforms/Utils/ValueMapper.h
>  lib/Linker/IRMover.cpp
>  lib/Transforms/Utils/ValueMapper.cpp
> 
> <D20593.58295.patch>

This is awesome.  LGTM with a couple of nits inline below.

> Index: lib/Transforms/Utils/ValueMapper.cpp
> ===================================================================
> --- lib/Transforms/Utils/ValueMapper.cpp
> +++ lib/Transforms/Utils/ValueMapper.cpp
> @@ -29,8 +29,6 @@
>  // Out of line method to get vtable etc for class.
>  void ValueMapTypeRemapper::anchor() {}
>  void ValueMaterializer::anchor() {}
> -void ValueMaterializer::materializeInitFor(GlobalValue *New, GlobalValue *Old) {
> -}
>  
>  namespace {
>  
> @@ -363,12 +361,8 @@
>  
>    // If we have a materializer and it can materialize a value, use that.
>    if (auto *Materializer = getMaterializer()) {
> -    if (Value *NewV =
> -            Materializer->materializeDeclFor(const_cast<Value *>(V))) {
> +    if (Value *NewV = Materializer->materialize(const_cast<Value *>(V))) {
>        getVM()[V] = NewV;
> -      if (auto *NewGV = dyn_cast<GlobalValue>(NewV))
> -        Materializer->materializeInitFor(
> -            NewGV, cast<GlobalValue>(const_cast<Value *>(V)));
>        return NewV;

I think the usual style is:
--
return getVM()[V] = NewV;
--

>      }
>    }
> Index: lib/Linker/IRMover.cpp
> ===================================================================
> --- lib/Linker/IRMover.cpp
> +++ lib/Linker/IRMover.cpp
> @@ -349,17 +349,15 @@
>  
>  public:
>    GlobalValueMaterializer(IRLinker &TheIRLinker) : TheIRLinker(TheIRLinker) {}
> -  Value *materializeDeclFor(Value *V) override;
> -  void materializeInitFor(GlobalValue *New, GlobalValue *Old) override;
> +  Value *materialize(Value *V) override;

I like this rename, but the commits would be easier to read if the
rename were separated.  Not a big deal if you think I'm wrong here, it's
NFC either way.

>  };