[llvm] r265631 - ValueMapper: Make LocalAsMetadata match function-local Values
Adrian Prantl via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 7 08:32:27 PDT 2016
> On Apr 6, 2016, at 8:42 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
> 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.
In addition to referencing a value before it has been defined, the position of the dbg.value intrinsic in the instruction stream is significant. Best example for this are constants.
>
> 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`.
That would work. I guess it is hard to impossible to catch a dbg.value referencing a constant being moved around.
>
> 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?)
This sounds like a generally good idea.
> - Should we change anything that calls RemapInstruction to do something
> special with debug info intrinsics? (Also reapply r265631, but no
> verifier check.)
What would that special thing be? The only thing we know is that the an intrinsic referencing a value must be dominated by that value, but otherwise we shouldn’t touch its relative position in the instruction stream.
>
> - Or should I re-implement r265631 more loosely, and we lose some debug
> info on inlining?
>
Unsurprisingly that sounds least compelling to me :-)
-- adrian
> <t.ll>
>
>> 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