<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 15, 2016 at 8:39 PM, Duncan P. N. Exon Smith via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Author: dexonsmith<br>
Date: Fri Apr 15 22:39:44 2016<br>
New Revision: 266513<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=266513&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=266513&view=rev</a><br>
Log:<br>
ValueMapper: Stop memoizing ConstantAsMetadata<br>
<br>
Stop memoizing ConstantAsMetadata in ValueMapper::mapMetadata. Now we<br>
have to recompute it, but these metadata aren't particularly common, and<br>
it restricts the lifetime of the Metadata map unnecessarily.<br>
<br>
(The motivation is that I have a patch which uses a single Metadata map<br>
for the lifetime of IRMover. Mehdi profiled r266446 with the patch<br>
applied and we saw a pretty big speedup in lib/Linker.)<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>
llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=266513&r1=266512&r2=266513&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=266513&r1=266512&r2=266513&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Fri Apr 15 22:39:44 2016<br>
@@ -505,14 +505,26 @@ bool MDNodeMapper::mapOperand(const Meta<br>
return false;<br>
<br>
if (Optional<Metadata *> MappedOp = M.mapSimpleMetadata(Op)) {<br>
- assert((isa<MDString>(Op) || M.getVM().getMappedMD(Op)) &&<br>
- "Expected result to be memoized");<br>
+ if (auto *CMD = dyn_cast<ConstantAsMetadata>(Op))<br></blockquote><div><br></div><div>CMD is unused in Release builds triggering a -Wunused-variable warning.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
+ assert((!*MappedOp || M.getVM().count(CMD->getValue()) ||<br>
+ M.getVM().getMappedMD(Op)) &&<br>
+ "Expected Value to be memoized");<br>
+ else<br>
+ assert((isa<MDString>(Op) || M.getVM().getMappedMD(Op)) &&<br>
+ "Expected result to be memoized");<br>
return *MappedOp != Op;<br>
}<br>
<br>
return push(*cast<MDNode>(Op)).HasChangedAddress;<br>
}<br>
<br>
+static ConstantAsMetadata *wrapConstantAsMetadata(const ConstantAsMetadata &CMD,<br>
+ Value *MappedV) {<br>
+ if (CMD.getValue() == MappedV)<br>
+ return const_cast<ConstantAsMetadata *>(&CMD);<br>
+ return MappedV ? ConstantAsMetadata::getConstant(MappedV) : nullptr;<br>
+}<br>
+<br>
Optional<Metadata *> MDNodeMapper::getMappedOp(const Metadata *Op) const {<br>
if (!Op)<br>
return nullptr;<br>
@@ -523,6 +535,9 @@ Optional<Metadata *> MDNodeMapper::getMa<br>
if (isa<MDString>(Op))<br>
return const_cast<Metadata *>(Op);<br>
<br>
+ if (auto *CMD = dyn_cast<ConstantAsMetadata>(Op))<br>
+ return wrapConstantAsMetadata(*CMD, M.getVM().lookup(CMD->getValue()));<br>
+<br>
return None;<br>
}<br>
<br>
@@ -720,6 +735,19 @@ Metadata *MDNodeMapper::map(const MDNode<br>
return *getMappedOp(&FirstN);<br>
}<br>
<br>
+namespace {<br>
+<br>
+struct MapMetadataDisabler {<br>
+ ValueToValueMapTy &VM;<br>
+<br>
+ MapMetadataDisabler(ValueToValueMapTy &VM) : VM(VM) {<br>
+ VM.disableMapMetadata();<br>
+ }<br>
+ ~MapMetadataDisabler() { VM.enableMapMetadata(); }<br>
+};<br>
+<br>
+} // end namespace<br>
+<br>
Optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {<br>
// If the value already exists in the map, use it.<br>
if (Optional<Metadata *> NewMD = getVM().getMappedMD(MD))<br>
@@ -735,14 +763,13 @@ Optional<Metadata *> Mapper::mapSimpleMe<br>
<br>
if (auto *CMD = dyn_cast<ConstantAsMetadata>(MD)) {<br>
// Disallow recursion into metadata mapping through mapValue.<br>
- getVM().disableMapMetadata();<br>
- Value *MappedV = mapValue(CMD->getValue());<br>
- getVM().enableMapMetadata();<br>
-<br>
- if (CMD->getValue() == MappedV)<br>
- return mapToSelf(MD);<br>
+ MapMetadataDisabler MMD(getVM());<br>
<br>
- return mapToMetadata(MD, MappedV ? ValueAsMetadata::get(MappedV) : nullptr);<br>
+ // Don't memoize ConstantAsMetadata. Instead of lasting until the<br>
+ // LLVMContext is destroyed, they can be deleted when the GlobalValue they<br>
+ // reference is destructed. These aren't super common, so the extra<br>
+ // indirection isn't that expensive.<br>
+ return wrapConstantAsMetadata(*CMD, mapValue(CMD->getValue()));<br>
}<br>
<br>
assert(isa<MDNode>(MD) && "Expected a metadata node");<br>
<br>
Modified: llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp?rev=266513&r1=266512&r2=266513&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp?rev=266513&r1=266512&r2=266513&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp (original)<br>
+++ llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp Fri Apr 15 22:39:44 2016<br>
@@ -238,13 +238,14 @@ TEST(ValueMapperTest, mapMetadataConstan<br>
<br>
auto *CAM = ConstantAsMetadata::get(F.get());<br>
{<br>
+ // ConstantAsMetadata shouldn't be memoized.<br>
ValueToValueMapTy VM;<br>
EXPECT_EQ(CAM, ValueMapper(VM).mapMetadata(*CAM));<br>
- EXPECT_TRUE(<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().count(CAM));<br>
- <a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().erase(CAM);<br>
+ EXPECT_FALSE(<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().count(CAM));<br>
EXPECT_EQ(CAM, ValueMapper(VM, RF_IgnoreMissingLocals).mapMetadata(*CAM));<br>
- EXPECT_TRUE(<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().count(CAM));<br>
+ EXPECT_FALSE(<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().count(CAM));<br>
<br>
+ // But it should respect a mapping that gets seeded.<br>
auto *N = MDTuple::get(C, None);<br>
<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>()[CAM].reset(N);<br>
EXPECT_EQ(N, ValueMapper(VM).mapMetadata(*CAM));<br>
@@ -256,7 +257,7 @@ TEST(ValueMapperTest, mapMetadataConstan<br>
ValueToValueMapTy VM;<br>
VM[F.get()] = F2.get();<br>
auto *F2MD = ValueMapper(VM).mapMetadata(*CAM);<br>
- EXPECT_TRUE(<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().count(CAM));<br>
+ EXPECT_FALSE(<a href="http://VM.MD" rel="noreferrer" target="_blank">VM.MD</a>().count(CAM));<br>
EXPECT_TRUE(F2MD);<br>
EXPECT_EQ(F2.get(), cast<ConstantAsMetadata>(F2MD)->getValue());<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">~Craig</div>
</div></div>