[llvm] r265631 - ValueMapper: Make LocalAsMetadata match function-local Values
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 7 17:41:31 PDT 2016
> On 2016-Apr-07, at 11:32, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> 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.
Filed PR27273.
>
>> - 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.
(I can't remember what precisely I was imagining.)
>> - Or should I re-implement r265631 more loosely, and we lose some debug
>> info on inlining?
>>
>
> Unsurprisingly that sounds least compelling to me :-)
r265759 does; it's a strict improvement, even if it doesn't fix the
debug info.
>
> -- 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