[llvm] r265631 - ValueMapper: Make LocalAsMetadata match function-local Values
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 6 20:42:41 PDT 2016
I could use some advice from debug info people. I reproduced this failure:
http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_build/18973/
It's kind of interesting. Reassociate is shuffling:
--
%or145 = or i64 %and142, %shl144, !dbg !340
tail call void @llvm.dbg.value(metadata i64 %or145, i64 0, metadata !65, metadata !104), !dbg !337
%or146 = or i64 %or145, %and58, !dbg !341
tail call void @llvm.dbg.value(metadata i64 %or146, i64 0, metadata !65, metadata !104), !dbg !337
--
into:
--
tail call void @llvm.dbg.value(metadata i64 %or145, i64 0, metadata !65, metadata !104), !dbg !337
%or145 = or i64 %and142, %and58, !dbg !340
%or146 = or i64 %or145, %shl144, !dbg !341
tail call void @llvm.dbg.value(metadata i64 %or146, i64 0, metadata !65, metadata !104), !dbg !337
--
The new order of instructions isn't really valid.
Ideally we'd add a verifier check for the new form, since %or145 is
referenced before it's defined. I assume this happens (and passes the
verifier) because the reference is through `metadata`.
In the inliner, CloneAndPruneIntoFromInst calls
PruningFunctionCloner::CloneBlock, which in turn calls
RemapInstruction on each instruction (skipping phis) in order. The
assertion goes off because we try to remap %or145 before it's actually
available in the following instruction.
As a concrete (slightly hand-rolled) example, I've attached t.ll, which
has @caller, which calls the following @foo:
--
define i32 @foo(i32 %i) #0 !dbg !4 {
entry:
%call = tail call i32 @sink(i32 %i) #3, !dbg !19
tail call void @llvm.dbg.value(metadata i32 %add, i64 0, metadata !9, metadata !17), !dbg !18
%add = add nsw i32 %call, %i, !dbg !20
ret i32 %add, !dbg !21
}
--
Inlining @foo into @caller crashes with r265631 because %add is
referenced before it's used.
Without r265631, it produces this:
--
define i32 @caller(i32 %i) #0 !dbg !10 {
entry:
tail call void @llvm.dbg.value(metadata i32 %i, i64 0, metadata !12, metadata !18), !dbg !22
%call.i = tail call i32 @sink(i32 %i) #3, !dbg !23
tail call void @llvm.dbg.value(metadata !2, i64 0, metadata !9, metadata !18) #3, !dbg !25
%add.i = add nsw i32 %call.i, %i, !dbg !26
ret i32 %add.i, !dbg !27
}
--
If the instructions had been properly ordered, we would have referenced
%add.i here instead of !{} (note that !2 = !{}).
EarlyCSE soon the @llvm.dbg.value entirely (not quite sure why, or
whether it's relevant).
Any thoughts on this?
- Should we enforce valid instruction ordering with a Verifier check
and fix the bugs? (Then I'd reapply r265631?)
- Should we change anything that calls RemapInstruction to do something
special with debug info intrinsics? (Also reapply r265631, but no
verifier check.)
- Or should I re-implement r265631 more loosely, and we lose some debug
info on inlining?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.ll
Type: application/octet-stream
Size: 3367 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160406/f8d7a5c7/attachment.obj>
-------------- next part --------------
> On 2016-Apr-06, at 19:22, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
> Reverted in r265637 due to bot failures:
> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-42vma/builds/7272
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/3256
> http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_build/18973
>
> The last one was in compiler-rt on Darwin, so I should be able to reproduce.
>
>> On 2016-Apr-06, at 18:08, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: dexonsmith
>> Date: Wed Apr 6 20:08:39 2016
>> New Revision: 265631
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=265631&view=rev
>> Log:
>> ValueMapper: Make LocalAsMetadata match function-local Values
>>
>> Start treating LocalAsMetadata similarly to function-local members of
>> the Value hierarchy in MapValue and MapMetadata.
>>
>> - Don't memoize them.
>> - Return nullptr if they are missing.
>>
>> This also cleans up ConstantAsMetadata to stop listening to the
>> RF_IgnoreMissingLocals flag.
>>
>> Modified:
>> llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>> llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=265631&r1=265630&r2=265631&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Wed Apr 6 20:08:39 2016
>> @@ -86,6 +86,9 @@ public:
>> /// (not an MDNode, or MDNode::isResolved() returns true).
>> Metadata *mapMetadata(const Metadata *MD);
>>
>> + // Map LocalAsMetadata, which never gets memoized.
>> + Metadata *mapLocalAsMetadata(const LocalAsMetadata &LAM);
>> +
>> private:
>> Value *mapBlockAddress(const BlockAddress &BA);
>>
>> @@ -300,18 +303,27 @@ Value *Mapper::mapValue(const Value *V)
>>
>> if (const auto *MDV = dyn_cast<MetadataAsValue>(V)) {
>> const Metadata *MD = MDV->getMetadata();
>> +
>> + // Locals shouldn't be memoized. Return nullptr if mapLocalAsMetadata()
>> + // returns nullptr; otherwise bridge back to the Value hierarchy.
>> + if (auto *LAM = dyn_cast<LocalAsMetadata>(MD)) {
>> + if (auto *MappedMD = mapLocalAsMetadata(*LAM)) {
>> + if (LAM == MappedMD)
>> + return const_cast<Value *>(V);
>> + return MetadataAsValue::get(V->getContext(), MappedMD);
>> + }
>> + return nullptr;
>> + }
>> +
>> // If this is a module-level metadata and we know that nothing at the module
>> // level is changing, then use an identity mapping.
>> - if (!isa<LocalAsMetadata>(MD) && (Flags & RF_NoModuleLevelChanges))
>> + if (Flags & RF_NoModuleLevelChanges)
>> return VM[V] = const_cast<Value *>(V);
>>
>> - // FIXME: be consistent with function-local values for LocalAsMetadata by
>> - // returning nullptr when LocalAsMetadata is missing. Adding a mapping is
>> - // expensive.
>> + // Map the metadata and turn it into a value.
>> auto *MappedMD = mapMetadata(MD);
>> - if (MD == MappedMD || (!MappedMD && (Flags & RF_IgnoreMissingLocals)))
>> + if (MD == MappedMD)
>> return VM[V] = const_cast<Value *>(V);
>> -
>> return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD);
>> }
>>
>> @@ -629,21 +641,16 @@ Optional<Metadata *> Mapper::mapSimpleMe
>> if (isa<MDString>(MD))
>> return mapToSelf(MD);
>>
>> - if (isa<ConstantAsMetadata>(MD))
>> + if (auto *CMD = dyn_cast<ConstantAsMetadata>(MD)) {
>> if ((Flags & RF_NoModuleLevelChanges))
>> return mapToSelf(MD);
>>
>> - // FIXME: Assert that this is not LocalAsMetadata. It should be handled
>> - // elsewhere.
>> - if (const auto *VMD = dyn_cast<ValueAsMetadata>(MD)) {
>> // Disallow recursion into metadata mapping through mapValue.
>> VM.disableMapMetadata();
>> - Value *MappedV = mapValue(VMD->getValue());
>> + Value *MappedV = mapValue(CMD->getValue());
>> VM.enableMapMetadata();
>>
>> - // FIXME: Always use "ignore" behaviour. There should only be globals here.
>> - if (VMD->getValue() == MappedV ||
>> - (!MappedV && (Flags & RF_IgnoreMissingLocals)))
>> + if (CMD->getValue() == MappedV)
>> return mapToSelf(MD);
>>
>> return mapToMetadata(MD, MappedV ? ValueAsMetadata::get(MappedV) : nullptr);
>> @@ -665,9 +672,24 @@ Metadata *llvm::MapMetadata(const Metada
>> return Mapper(VM, Flags, TypeMapper, Materializer).mapMetadata(MD);
>> }
>>
>> +Metadata *Mapper::mapLocalAsMetadata(const LocalAsMetadata &LAM) {
>> + if (Optional<Metadata *> NewMD = VM.getMappedMD(&LAM))
>> + return *NewMD;
>> +
>> + // Lookup the mapping for the value itself, and return the appropriate
>> + // metadata.
>> + if (Value *V = mapValue(LAM.getValue())) {
>> + if (V == LAM.getValue())
>> + return const_cast<LocalAsMetadata *>(&LAM);
>> + return ValueAsMetadata::get(V);
>> + }
>> + return nullptr;
>> +}
>> +
>> Metadata *Mapper::mapMetadata(const Metadata *MD) {
>> - // FIXME: First check for and deal with LocalAsMetadata, so that
>> - // mapSimpleMetadata() doesn't need to deal with it.
>> + if (auto *LAM = dyn_cast<LocalAsMetadata>(MD))
>> + return mapLocalAsMetadata(*LAM);
>> +
>> if (Optional<Metadata *> NewMD = mapSimpleMetadata(MD))
>> return *NewMD;
>>
>>
>> Modified: llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp?rev=265631&r1=265630&r2=265631&view=diff
>> ==============================================================================
>> --- llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp (original)
>> +++ llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp Wed Apr 6 20:08:39 2016
>> @@ -7,6 +7,7 @@
>> //
>> //===----------------------------------------------------------------------===//
>>
>> +#include "llvm/IR/Function.h"
>> #include "llvm/IR/LLVMContext.h"
>> #include "llvm/IR/Metadata.h"
>> #include "llvm/Transforms/Utils/ValueMapper.h"
>> @@ -126,4 +127,86 @@ TEST(ValueMapperTest, MapMetadataSeededW
>> EXPECT_EQ(nullptr, MapMetadata(D, VM, RF_None));
>> }
>>
>> +TEST(ValueMapperTest, MapMetadataLocalAsMetadata) {
>> + LLVMContext C;
>> + FunctionType *FTy =
>> + FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false);
>> + std::unique_ptr<Function> F(
>> + Function::Create(FTy, GlobalValue::ExternalLinkage, "F"));
>> + Argument &A = *F->arg_begin();
>> +
>> + auto *LAM = LocalAsMetadata::get(&A);
>> + ValueToValueMapTy VM;
>> + EXPECT_EQ(nullptr, MapMetadata(LAM, VM));
>> + EXPECT_EQ(nullptr, MapMetadata(LAM, VM, RF_IgnoreMissingLocals));
>> + EXPECT_EQ(None, VM.getMappedMD(LAM));
>> +
>> + VM.MD()[LAM].reset(LAM);
>> + EXPECT_EQ(LAM, MapMetadata(LAM, VM));
>> + EXPECT_EQ(LAM, MapMetadata(LAM, VM, RF_IgnoreMissingLocals));
>> +
>> + auto *N = MDNode::get(C, None);
>> + VM.MD()[LAM].reset(N);
>> + EXPECT_EQ(N, MapMetadata(LAM, VM));
>> + EXPECT_EQ(N, MapMetadata(LAM, VM, RF_IgnoreMissingLocals));
>> +}
>> +
>> +TEST(ValueMapperTest, MapMetadataConstantAsMetadata) {
>> + LLVMContext C;
>> + FunctionType *FTy =
>> + FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false);
>> + std::unique_ptr<Function> F(
>> + Function::Create(FTy, GlobalValue::ExternalLinkage, "F"));
>> +
>> + auto *CAM = ConstantAsMetadata::get(F.get());
>> + {
>> + ValueToValueMapTy VM;
>> + EXPECT_EQ(CAM, MapMetadata(CAM, VM));
>> + EXPECT_TRUE(VM.MD().count(CAM));
>> + VM.MD().erase(CAM);
>> + EXPECT_EQ(CAM, MapMetadata(CAM, VM, RF_IgnoreMissingLocals));
>> + EXPECT_TRUE(VM.MD().count(CAM));
>> +
>> + auto *N = MDNode::get(C, None);
>> + VM.MD()[CAM].reset(N);
>> + EXPECT_EQ(N, MapMetadata(CAM, VM));
>> + EXPECT_EQ(N, MapMetadata(CAM, VM, RF_IgnoreMissingLocals));
>> + }
>> +
>> + std::unique_ptr<Function> F2(
>> + Function::Create(FTy, GlobalValue::ExternalLinkage, "F2"));
>> + ValueToValueMapTy VM;
>> + VM[F.get()] = F2.get();
>> + auto *F2MD = MapMetadata(CAM, VM);
>> + EXPECT_TRUE(VM.MD().count(CAM));
>> + EXPECT_TRUE(F2MD);
>> + EXPECT_EQ(F2.get(), cast<ConstantAsMetadata>(F2MD)->getValue());
>> +}
>> +
>> +TEST(ValueMapperTest, MapValueLocalAsMetadata) {
>> + LLVMContext C;
>> + FunctionType *FTy =
>> + FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false);
>> + std::unique_ptr<Function> F(
>> + Function::Create(FTy, GlobalValue::ExternalLinkage, "F"));
>> + Argument &A = *F->arg_begin();
>> +
>> + auto *LAM = LocalAsMetadata::get(&A);
>> + auto *MAV = MetadataAsValue::get(C, LAM);
>> +
>> + ValueToValueMapTy VM;
>> + EXPECT_EQ(nullptr, MapValue(MAV, VM));
>> + EXPECT_EQ(nullptr, MapValue(MAV, VM, RF_IgnoreMissingLocals));
>> + EXPECT_FALSE(VM.count(MAV));
>> + EXPECT_FALSE(VM.count(&A));
>> +
>> + VM[MAV] = MAV;
>> + EXPECT_EQ(MAV, MapValue(MAV, VM));
>> + EXPECT_EQ(MAV, MapValue(MAV, VM, RF_IgnoreMissingLocals));
>> +
>> + VM[MAV] = &A;
>> + EXPECT_EQ(&A, MapValue(MAV, VM));
>> + EXPECT_EQ(&A, MapValue(MAV, VM, RF_IgnoreMissingLocals));
>> +}
>> +
>> } // end namespace
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list