From b9e70ff6a8633e487bf0c020a31f4eb78c2d4346 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Sat, 8 May 2021 17:05:05 +0200 Subject: [PATCH] [Inliner] Fix noalias metadata handling for instructions simplified during cloning (PR50270) Instead of using VMap, which may include instructions from the caller as a result of simplification, iterate over the (FirstNewBlock, Caller->end()) range, which will only include new instructions. Fixes https://bugs.llvm.org/show_bug.cgi?id=50270. Differential Revision: https://reviews.llvm.org/D102110 --- lib/Transforms/Utils/InlineFunction.cpp | 106 ++++++++++-------------- test/Transforms/Inline/pr50270.ll | 71 ++++++++++++++++ 2 files changed, 117 insertions(+), 60 deletions(-) create mode 100644 test/Transforms/Inline/pr50270.ll diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp index 201e4e1c58d..be4c8281167 100644 --- a/lib/Transforms/Utils/InlineFunction.cpp +++ b/lib/Transforms/Utils/InlineFunction.cpp @@ -782,7 +782,8 @@ static void HandleInlinedEHPad(InvokeInst *II, BasicBlock *FirstNewBlock, /// When inlining a call site that has !llvm.mem.parallel_loop_access, /// !llvm.access.group, !alias.scope or !noalias metadata, that metadata should /// be propagated to all memory-accessing cloned instructions. -static void PropagateCallSiteMetadata(CallBase &CB, ValueToValueMapTy &VMap) { +static void PropagateCallSiteMetadata(CallBase &CB, Function::iterator FStart, + Function::iterator FEnd) { MDNode *MemParallelLoopAccess = CB.getMetadata(LLVMContext::MD_mem_parallel_loop_access); MDNode *AccessGroup = CB.getMetadata(LLVMContext::MD_access_group); @@ -791,41 +792,33 @@ static void PropagateCallSiteMetadata(CallBase &CB, ValueToValueMapTy &VMap) { if (!MemParallelLoopAccess && !AccessGroup && !AliasScope && !NoAlias) return; - for (ValueToValueMapTy::iterator VMI = VMap.begin(), VMIE = VMap.end(); - VMI != VMIE; ++VMI) { - // Check that key is an instruction, to skip the Argument mapping, which - // points to an instruction in the original function, not the inlined one. - if (!VMI->second || !isa(VMI->first)) - continue; + for (BasicBlock &BB : make_range(FStart, FEnd)) { + for (Instruction &I : BB) { + // This metadata is only relevant for instructions that access memory. + if (!I.mayReadOrWriteMemory()) + continue; - Instruction *NI = dyn_cast(VMI->second); - if (!NI) - continue; - - // This metadata is only relevant for instructions that access memory. - if (!NI->mayReadOrWriteMemory()) - continue; - - if (MemParallelLoopAccess) { - // TODO: This probably should not overwrite MemParalleLoopAccess. - MemParallelLoopAccess = MDNode::concatenate( - NI->getMetadata(LLVMContext::MD_mem_parallel_loop_access), - MemParallelLoopAccess); - NI->setMetadata(LLVMContext::MD_mem_parallel_loop_access, + if (MemParallelLoopAccess) { + // TODO: This probably should not overwrite MemParalleLoopAccess. + MemParallelLoopAccess = MDNode::concatenate( + I.getMetadata(LLVMContext::MD_mem_parallel_loop_access), + MemParallelLoopAccess); + I.setMetadata(LLVMContext::MD_mem_parallel_loop_access, MemParallelLoopAccess); + } + + if (AccessGroup) + I.setMetadata(LLVMContext::MD_access_group, uniteAccessGroups( + I.getMetadata(LLVMContext::MD_access_group), AccessGroup)); + + if (AliasScope) + I.setMetadata(LLVMContext::MD_alias_scope, MDNode::concatenate( + I.getMetadata(LLVMContext::MD_alias_scope), AliasScope)); + + if (NoAlias) + I.setMetadata(LLVMContext::MD_noalias, MDNode::concatenate( + I.getMetadata(LLVMContext::MD_noalias), NoAlias)); } - - if (AccessGroup) - NI->setMetadata(LLVMContext::MD_access_group, uniteAccessGroups( - NI->getMetadata(LLVMContext::MD_access_group), AccessGroup)); - - if (AliasScope) - NI->setMetadata(LLVMContext::MD_alias_scope, MDNode::concatenate( - NI->getMetadata(LLVMContext::MD_alias_scope), AliasScope)); - - if (NoAlias) - NI->setMetadata(LLVMContext::MD_noalias, MDNode::concatenate( - NI->getMetadata(LLVMContext::MD_noalias), NoAlias)); } } @@ -846,9 +839,9 @@ public: /// subsequent remap() calls. void clone(); - /// Remap instructions in the given VMap from the original to the cloned + /// Remap instructions in the given range from the original to the cloned /// metadata. - void remap(ValueToValueMapTy &VMap); + void remap(Function::iterator FStart, Function::iterator FEnd); }; ScopedAliasMetadataDeepCloner::ScopedAliasMetadataDeepCloner( @@ -909,34 +902,27 @@ void ScopedAliasMetadataDeepCloner::clone() { } } -void ScopedAliasMetadataDeepCloner::remap(ValueToValueMapTy &VMap) { +void ScopedAliasMetadataDeepCloner::remap(Function::iterator FStart, + Function::iterator FEnd) { if (MDMap.empty()) return; // Nothing to do. - for (auto Entry : VMap) { - // Check that key is an instruction, to skip the Argument mapping, which - // points to an instruction in the original function, not the inlined one. - if (!Entry->second || !isa(Entry->first)) - continue; + for (BasicBlock &BB : make_range(FStart, FEnd)) { + for (Instruction &I : BB) { + // TODO: The null checks for the MDMap.lookup() results should no longer + // be necessary. + if (MDNode *M = I.getMetadata(LLVMContext::MD_alias_scope)) + if (MDNode *MNew = MDMap.lookup(M)) + I.setMetadata(LLVMContext::MD_alias_scope, MNew); - Instruction *I = dyn_cast(Entry->second); - if (!I) - continue; + if (MDNode *M = I.getMetadata(LLVMContext::MD_noalias)) + if (MDNode *MNew = MDMap.lookup(M)) + I.setMetadata(LLVMContext::MD_noalias, MNew); - // Only update scopes when we find them in the map. If they are not, it is - // because we already handled that instruction before. This is faster than - // tracking which instructions we already updated. - if (MDNode *M = I->getMetadata(LLVMContext::MD_alias_scope)) - if (MDNode *MNew = MDMap.lookup(M)) - I->setMetadata(LLVMContext::MD_alias_scope, MNew); - - if (MDNode *M = I->getMetadata(LLVMContext::MD_noalias)) - if (MDNode *MNew = MDMap.lookup(M)) - I->setMetadata(LLVMContext::MD_noalias, MNew); - - if (auto *Decl = dyn_cast(I)) - if (MDNode *MNew = MDMap.lookup(Decl->getScopeList())) - Decl->setScopeList(MNew); + if (auto *Decl = dyn_cast(&I)) + if (MDNode *MNew = MDMap.lookup(Decl->getScopeList())) + Decl->setScopeList(MNew); + } } } @@ -2038,7 +2024,7 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI, // Now clone the inlined noalias scope metadata. SAMetadataCloner.clone(); - SAMetadataCloner.remap(VMap); + SAMetadataCloner.remap(FirstNewBlock, Caller->end()); // Add noalias metadata if necessary. AddAliasScopeMetadata(CB, VMap, DL, CalleeAAR); @@ -2048,7 +2034,7 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI, AddReturnAttributes(CB, VMap); // Propagate metadata on the callsite if necessary. - PropagateCallSiteMetadata(CB, VMap); + PropagateCallSiteMetadata(CB, FirstNewBlock, Caller->end()); // Register any cloned assumptions. if (IFI.GetAssumptionCache) diff --git a/test/Transforms/Inline/pr50270.ll b/test/Transforms/Inline/pr50270.ll new file mode 100644 index 00000000000..be7c3379ce8 --- /dev/null +++ b/test/Transforms/Inline/pr50270.ll @@ -0,0 +1,71 @@ +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py +; RUN: opt -S -inline < %s | FileCheck %s + +; This tests cases where instructions in the callee are simplified to +; instructions in the caller, thus making VMap contain instructions from +; the caller. We should not be assigning incorrect noalias metadata in +; that case. + +declare { i64* } @opaque_callee() + +define { i64* } @callee(i64* %x) { +; CHECK-LABEL: @callee( +; CHECK-NEXT: [[RES:%.*]] = insertvalue { i64* } undef, i64* [[X:%.*]], 0 +; CHECK-NEXT: ret { i64* } [[RES]] +; + %res = insertvalue { i64* } undef, i64* %x, 0 + ret { i64* } %res +} + +; @opaque_callee() should not receive noalias metadata here. +define void @caller() { +; CHECK-LABEL: @caller( +; CHECK-NEXT: call void @llvm.experimental.noalias.scope.decl(metadata !0) +; CHECK-NEXT: [[S:%.*]] = call { i64* } @opaque_callee() +; CHECK-NEXT: [[X:%.*]] = extractvalue { i64* } [[S]], 0 +; CHECK-NEXT: ret void +; + call void @llvm.experimental.noalias.scope.decl(metadata !0) + %s = call { i64* } @opaque_callee() + %x = extractvalue { i64* } %s, 0 + call { i64* } @callee(i64* %x), !noalias !0 + ret void +} + +; @opaque_callee() should no the same noalias metadata as the load from the +; else branch, not as the load in the if branch. +define { i64* } @self_caller(i1 %c, i64* %a) { +; CHECK-LABEL: @self_caller( +; CHECK-NEXT: call void @llvm.experimental.noalias.scope.decl(metadata !0) +; CHECK-NEXT: br i1 [[C:%.*]], label [[IF:%.*]], label [[ELSE:%.*]] +; CHECK: if: +; CHECK-NEXT: [[S:%.*]] = call { i64* } @opaque_callee(), !noalias !0 +; CHECK-NEXT: [[X:%.*]] = extractvalue { i64* } [[S]], 0 +; CHECK-NEXT: call void @llvm.experimental.noalias.scope.decl(metadata !3) +; CHECK-NEXT: [[TMP1:%.*]] = load volatile i64, i64* [[X]], align 4, !alias.scope !3 +; CHECK-NEXT: ret { i64* } [[S]] +; CHECK: else: +; CHECK-NEXT: [[R2:%.*]] = insertvalue { i64* } undef, i64* [[A:%.*]], 0 +; CHECK-NEXT: [[TMP2:%.*]] = load volatile i64, i64* [[A]], align 4, !alias.scope !0 +; CHECK-NEXT: ret { i64* } [[R2]] +; + call void @llvm.experimental.noalias.scope.decl(metadata !0) + br i1 %c, label %if, label %else + +if: + %s = call { i64* } @opaque_callee(), !noalias !0 + %x = extractvalue { i64* } %s, 0 + %r = call { i64* } @self_caller(i1 false, i64* %x) + ret { i64* } %r + +else: + %r2 = insertvalue { i64* } undef, i64* %a, 0 + load volatile i64, i64* %a, !alias.scope !0 + ret { i64* } %r2 +} + +declare void @llvm.experimental.noalias.scope.decl(metadata) + +!0 = !{!1} +!1 = !{!1, !2, !"scope"} +!2 = !{!2, !"domain"}