[PATCH] D20593: ValueMaterializer: fuse materializeDeclFor and materializeInitFor (NFC)
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Tue May 24 14:13:01 PDT 2016
> 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.
> };
More information about the llvm-commits
mailing list