From da68cbc4ad104a98182d44bb66aa08048f2cc061 Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Thu, 7 Apr 2016 00:26:43 +0000 Subject: [PATCH] IR: RF_IgnoreMissingValues => RF_IgnoreMissingLocals, NFC Clarify what this RemapFlag actually means. - Change the flag name to match its intended behaviour. - Clearly document that it's not supposed to affect globals. - Add a host of FIXMEs to indicate how to fix the behaviour to match the intent of the flag. RF_IgnoreMissingLocals should only affect the behaviour of RemapInstruction for function-local operands; namely, for operands of type Argument, Instruction, and BasicBlock. Currently, it is *only* passed into RemapInstruction calls (and the transitive MapValue calls that it makes). When I split Metadata from Value I didn't understand the flag, and I used it in a bunch of places for "global" metadata. This commit doesn't have any functionality change, but prepares to cleanup MapMetadata and MapValue. llvm-svn: 265628 --- llvm/include/llvm/Transforms/Utils/ValueMapper.h | 23 +++++++++++++++++++--- llvm/lib/CodeGen/WinEHPrepare.cpp | 2 +- llvm/lib/Linker/IRMover.cpp | 2 +- .../Scalar/InductiveRangeCheckElimination.cpp | 2 +- llvm/lib/Transforms/Scalar/LoopRotation.cpp | 2 +- llvm/lib/Transforms/Scalar/LoopUnswitch.cpp | 2 +- llvm/lib/Transforms/Utils/CloneFunction.cpp | 2 +- llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | 2 +- llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 4 ++-- llvm/lib/Transforms/Utils/ValueMapper.cpp | 22 +++++++++++++++------ 10 files changed, 45 insertions(+), 18 deletions(-) diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h index 315de64..f986c01 100644 --- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h +++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h @@ -67,17 +67,22 @@ enum RemapFlags { /// values like functions and global metadata. RF_NoModuleLevelChanges = 1, - /// If this flag is set, the remapper ignores entries that are not in the + /// If this flag is set, the remapper ignores missing function-local entries + /// (Argument, Instruction, BasicBlock) that are not in the /// value map. If it is unset, it aborts if an operand is asked to be /// remapped which doesn't exist in the mapping. - RF_IgnoreMissingEntries = 2, + /// + /// There are no such assertions in MapValue(), whose result should be + /// essentially unchanged by this flag. This only changes the assertion + /// behaviour in RemapInstruction(). + RF_IgnoreMissingLocals = 2, /// Instruct the remapper to move distinct metadata instead of duplicating it /// when there are module-level changes. RF_MoveDistinctMDs = 4, /// Any global values not in value map are mapped to null instead of mapping - /// to self. Illegal if RF_IgnoreMissingEntries is also set. + /// to self. Illegal if RF_IgnoreMissingLocals is also set. RF_NullMapMissingGlobalValues = 8, }; @@ -85,6 +90,18 @@ static inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) { return RemapFlags(unsigned(LHS) | unsigned(RHS)); } +/// Look up or compute a value in the value map. +/// +/// Return a mapped value for a function-local value (Argument, Instruction, +/// BasicBlock), or compute and memoize a value for a Constant. +/// +/// 1. If \c V is in VM, return the result. +/// 2. Else if \c V can be materialized with \c Materializer, do so, memoize +/// it in \c VM, and return it. +/// 3. Else if \c V is a function-local value, return nullptr. +/// 4. Else if \c V is a \a GlobalValue, return \c nullptr or \c V depending +/// on \a RF_NullMapMissingGlobalValues. +/// 5. Else, Compute the equivalent constant, and return it. Value *MapValue(const Value *V, ValueToValueMapTy &VM, RemapFlags Flags = RF_None, ValueMapTypeRemapper *TypeMapper = nullptr, diff --git a/llvm/lib/CodeGen/WinEHPrepare.cpp b/llvm/lib/CodeGen/WinEHPrepare.cpp index b2b3130..041fb7b 100644 --- a/llvm/lib/CodeGen/WinEHPrepare.cpp +++ b/llvm/lib/CodeGen/WinEHPrepare.cpp @@ -787,7 +787,7 @@ void WinEHPrepare::cloneCommonBlocks(Function &F) { // Loop over all instructions, fixing each one as we find it... for (Instruction &I : *BB) RemapInstruction(&I, VMap, - RF_IgnoreMissingEntries | RF_NoModuleLevelChanges); + RF_IgnoreMissingLocals | RF_NoModuleLevelChanges); // Catchrets targeting cloned blocks need to be updated separately from // the loop above because they are not in the current funclet. diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp index 5351bd5..2958d96 100644 --- a/llvm/lib/Linker/IRMover.cpp +++ b/llvm/lib/Linker/IRMover.cpp @@ -997,7 +997,7 @@ bool IRLinker::linkFunctionBody(Function &Dst, Function &Src) { A.mutateType(TypeMap.get(A.getType())); for (BasicBlock &BB : Dst) for (Instruction &I : BB) - RemapInstruction(&I, ValueMap, RF_IgnoreMissingEntries | ValueMapperFlags, + RemapInstruction(&I, ValueMap, RF_IgnoreMissingLocals | ValueMapperFlags, &TypeMap, &GValMaterializer); return false; diff --git a/llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp b/llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp index 48ec2de..8f283f9 100644 --- a/llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp +++ b/llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp @@ -939,7 +939,7 @@ void LoopConstrainer::cloneLoop(LoopConstrainer::ClonedLoop &Result, for (Instruction &I : *ClonedBB) RemapInstruction(&I, Result.Map, - RF_NoModuleLevelChanges | RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); // Exit blocks will now have one more predecessor and their PHI nodes need // to be edited to reflect that. No phi nodes need to be introduced because diff --git a/llvm/lib/Transforms/Scalar/LoopRotation.cpp b/llvm/lib/Transforms/Scalar/LoopRotation.cpp index 1a9d1db..b814891 100644 --- a/llvm/lib/Transforms/Scalar/LoopRotation.cpp +++ b/llvm/lib/Transforms/Scalar/LoopRotation.cpp @@ -244,7 +244,7 @@ static bool rotateLoop(Loop *L, unsigned MaxHeaderSize, LoopInfo *LI, // Eagerly remap the operands of the instruction. RemapInstruction(C, ValueMap, - RF_NoModuleLevelChanges|RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); // With the operands remapped, see if the instruction constant folds or is // otherwise simplifyable. This commonly occurs because the entry from PHI diff --git a/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp index c3b941f..955604f 100644 --- a/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp @@ -1068,7 +1068,7 @@ void LoopUnswitch::UnswitchNontrivialCondition(Value *LIC, Constant *Val, for (BasicBlock::iterator I = NewBlocks[i]->begin(), E = NewBlocks[i]->end(); I != E; ++I) RemapInstruction(&*I, VMap, - RF_NoModuleLevelChanges | RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); // Rewrite the original preheader to select between versions of the loop. BranchInst *OldBR = cast(loopPreheader->getTerminator()); diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp index 8e1715a..cf39ae9 100644 --- a/llvm/lib/Transforms/Utils/CloneFunction.cpp +++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp @@ -694,7 +694,7 @@ void llvm::remapInstructionsInBlocks( for (auto *BB : Blocks) for (auto &Inst : *BB) RemapInstruction(&Inst, VMap, - RF_NoModuleLevelChanges | RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); } /// \brief Clones a loop \p OrigLoop. Returns the loop and the blocks in \p diff --git a/llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp b/llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp index 5e12854..8f32e00 100644 --- a/llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp +++ b/llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp @@ -636,7 +636,7 @@ bool llvm::UnrollRuntimeLoopRemainder(Loop *L, unsigned Count, for (BasicBlock *BB : NewBlocks) { for (Instruction &I : *BB) { RemapInstruction(&I, VMap, - RF_NoModuleLevelChanges | RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); } } diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index f750509..8284d6b 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -2236,7 +2236,7 @@ bool llvm::FoldBranchToCommonDest(BranchInst *BI, unsigned BonusInstThreshold) { continue; Instruction *NewBonusInst = BonusInst->clone(); RemapInstruction(NewBonusInst, VMap, - RF_NoModuleLevelChanges | RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); VMap[&*BonusInst] = NewBonusInst; // If we moved a load, we cannot any longer claim any knowledge about @@ -2255,7 +2255,7 @@ bool llvm::FoldBranchToCommonDest(BranchInst *BI, unsigned BonusInstThreshold) { // two conditions together. Instruction *New = Cond->clone(); RemapInstruction(New, VMap, - RF_NoModuleLevelChanges | RF_IgnoreMissingEntries); + RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); PredBlock->getInstList().insert(PBI->getIterator(), New); New->takeName(Cond); Cond->setName(New->getName() + ".old"); diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp index 0c2954f..25c90b1 100644 --- a/llvm/lib/Transforms/Utils/ValueMapper.cpp +++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp @@ -274,9 +274,11 @@ Value *Mapper::mapValue(const Value *V) { // are using the identity mapping. if (isa(V)) { if (Flags & RF_NullMapMissingGlobalValues) { - assert(!(Flags & RF_IgnoreMissingEntries) && + // FIXME: Remove this assertion. RF_IgnoreMissingLocals is unrelated to + // RF_NullMapMissingGlobalValues. + assert(!(Flags & RF_IgnoreMissingLocals) && "Illegal to specify both RF_NullMapMissingGlobalValues and " - "RF_IgnoreMissingEntries"); + "RF_IgnoreMissingLocals"); return nullptr; } return VM[V] = const_cast(V); @@ -303,8 +305,11 @@ Value *Mapper::mapValue(const Value *V) { if (!isa(MD) && (Flags & RF_NoModuleLevelChanges)) return VM[V] = const_cast(V); + // FIXME: be consistent with function-local values for LocalAsMetadata by + // returning nullptr when LocalAsMetadata is missing. Adding a mapping is + // expensive. auto *MappedMD = mapMetadata(MD); - if (MD == MappedMD || (!MappedMD && (Flags & RF_IgnoreMissingEntries))) + if (MD == MappedMD || (!MappedMD && (Flags & RF_IgnoreMissingLocals))) return VM[V] = const_cast(V); return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD); @@ -628,14 +633,17 @@ Optional Mapper::mapSimpleMetadata(const Metadata *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(MD)) { // Disallow recursion into metadata mapping through mapValue. VM.disableMapMetadata(); Value *MappedV = mapValue(VMD->getValue()); VM.enableMapMetadata(); + // FIXME: Always use "ignore" behaviour. There should only be globals here. if (VMD->getValue() == MappedV || - (!MappedV && (Flags & RF_IgnoreMissingEntries))) + (!MappedV && (Flags & RF_IgnoreMissingLocals))) return mapToSelf(MD); return mapToMetadata(MD, MappedV ? ValueAsMetadata::get(MappedV) : nullptr); @@ -658,6 +666,8 @@ Metadata *llvm::MapMetadata(const Metadata *MD, ValueToValueMapTy &VM, } 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 (Optional NewMD = mapSimpleMetadata(MD)) return *NewMD; @@ -703,7 +713,7 @@ void llvm::RemapInstruction(Instruction *I, ValueToValueMapTy &VMap, if (V) *op = V; else - assert((Flags & RF_IgnoreMissingEntries) && + assert((Flags & RF_IgnoreMissingLocals) && "Referenced value not in value map!"); } @@ -715,7 +725,7 @@ void llvm::RemapInstruction(Instruction *I, ValueToValueMapTy &VMap, if (V) PN->setIncomingBlock(i, cast(V)); else - assert((Flags & RF_IgnoreMissingEntries) && + assert((Flags & RF_IgnoreMissingLocals) && "Referenced block not in value map!"); } } -- 2.7.4