[llvm] 451799b - [IRMover] Remove UB implying parameter attributes when necessary
Benjamin Kramer via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 21 09:43:36 PST 2023
Author: Tim Neumann
Date: 2023-02-21T18:35:31+01:00
New Revision: 451799bb8261bde52bbfef226d019caf1d82aa42
URL: https://github.com/llvm/llvm-project/commit/451799bb8261bde52bbfef226d019caf1d82aa42
DIFF: https://github.com/llvm/llvm-project/commit/451799bb8261bde52bbfef226d019caf1d82aa42.diff
LOG: [IRMover] Remove UB implying parameter attributes when necessary
When importing functions from some module X into some module Y, they may reference other functions already present in Y. The signature (especially attributes) of those other functions may have diverged between X and Y (e.g. due to the Dead Argument Elimination optimization). If necessary, modify the attributes to avoid UB.
See the added test and implementation comments for more details.
This was exposed by https://reviews.llvm.org/D133036 before it was reverted. Fixes https://github.com/llvm/llvm-project/issues/58976.
Reviewed By: tejohnson
Differential Revision: https://reviews.llvm.org/D139209
Added:
llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll
llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll
Modified:
llvm/include/llvm/IR/Attributes.h
llvm/lib/Linker/IRMover.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index c4e12a673ed23..fa179c9d367ea 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/llvm/include/llvm/IR/Attributes.h
@@ -360,6 +360,13 @@ class AttributeSet {
/// Return true if the attribute exists in this set.
bool hasAttribute(StringRef Kind) const;
+ /// Return true if the attribute exists in this set.
+ bool hasAttribute(Attribute A) const {
+ if (A.isStringAttribute())
+ return hasAttribute(A.getKindAsString());
+ return hasAttribute(A.getKindAsEnum());
+ }
+
/// Return the attribute object.
Attribute getAttribute(Attribute::AttrKind Kind) const;
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 721acb1ea02ca..fc226302423cf 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -529,7 +529,7 @@ class IRLinker {
void linkNamedMDNodes();
/// Update attributes while linking.
- void updateAttributes(GlobalValue &GV);
+ void updateAttributes(GlobalValue &DstGV, GlobalValue &SrcGV);
public:
IRLinker(Module &DstM, MDMapT &SharedMDs,
@@ -641,7 +641,7 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
if (ForIndirectSymbol || shouldLink(New, *SGV))
setError(linkGlobalValueBody(*New, *SGV));
- updateAttributes(*New);
+ updateAttributes(*New, *SGV);
return New;
}
@@ -1533,7 +1533,10 @@ static std::string adjustInlineAsm(const std::string &InlineAsm,
return InlineAsm;
}
-void IRLinker::updateAttributes(GlobalValue &GV) {
+void IRLinker::updateAttributes(GlobalValue &DstGV, GlobalValue &SrcGV) {
+ auto *DstF = dyn_cast<Function>(&DstGV);
+ auto *SrcF = dyn_cast<Function>(&SrcGV);
+
/// Remove nocallback attribute while linking, because nocallback attribute
/// indicates that the function is only allowed to jump back into caller's
/// module only by a return or an exception. When modules are linked, this
@@ -1548,16 +1551,53 @@ void IRLinker::updateAttributes(GlobalValue &GV) {
/// participate in the LTO link, and thus ends up in the merged module
/// containing its caller and callee, removing the attribute doesn't hurt as
/// it has no effect on definitions in the same module.
- if (auto *F = dyn_cast<Function>(&GV)) {
- if (!F->isIntrinsic())
- F->removeFnAttr(llvm::Attribute::NoCallback);
+ if (DstF) {
+ if (!DstF->isIntrinsic())
+ DstF->removeFnAttr(llvm::Attribute::NoCallback);
// Remove nocallback attribute when it is on a call-site.
- for (BasicBlock &BB : *F)
+ for (BasicBlock &BB : *DstF)
for (Instruction &I : BB)
if (CallBase *CI = dyn_cast<CallBase>(&I))
CI->removeFnAttr(Attribute::NoCallback);
}
+
+ // Fix compatibility of diverged function signatures.
+ //
+ // If `F` was a definition in its source module but is (already) a
+ // declaration in the destination module, then the signatures of the two
+ // functions may have diverged (even if they were originally idential) due to
+ // optimizations performed on the source module.
+ //
+ // See the test in `FunctionImport/attr_fixup_dae_noundef.ll` for an example
+ // based on how Dead Argument Elimination can remove `noundef` from function
+ // arguments.
+ //
+ // The code below handles _known_ incompatibilities. Others may exist.
+ if (DstF && SrcF && DstF->isDeclaration() && !SrcF->isDeclaration()) {
+ assert(DstF->arg_size() == SrcF->arg_size() &&
+ "Dst and Src should have the same signature.");
+
+ // Remove UB implying attributes present on `Dst`'s arguments but not
+ // `Src`'s.
+ AttributeMask UBImplyingAttributes =
+ AttributeFuncs::getUBImplyingAttributes();
+ for (size_t ArgI = 0; ArgI < DstF->arg_size(); ArgI++) {
+ AttributeSet DstAttrs = DstF->getAttributes().getParamAttrs(ArgI);
+ AttributeSet SrcAttrs = SrcF->getAttributes().getParamAttrs(ArgI);
+ AttributeMask ToRemove;
+
+ for (auto &Attr : DstAttrs) {
+ if (SrcAttrs.hasAttribute(Attr))
+ continue;
+
+ if (UBImplyingAttributes.contains(Attr))
+ ToRemove.addAttribute(Attr);
+ }
+
+ DstF->removeParamAttrs(ArgI, ToRemove);
+ }
+ }
}
Error IRLinker::run() {
diff --git a/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll b/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll
new file mode 100644
index 0000000000000..c8e31217633d5
--- /dev/null
+++ b/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll
@@ -0,0 +1,17 @@
+; This file contains the post-"deadargelim" IR.
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @outer(i32 noundef %arg) {
+ ; The parameter was originally `noundef %arg`, changed to `poison` by "deadargelim".
+ call void @inner(i32 poison)
+ ret void
+}
+
+; %arg was originally `noundef`, removed by "deadargelim".
+define void @inner(i32 %arg) #0 {
+ ret void
+}
+
+attributes #0 = { noinline }
diff --git a/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll b/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll
new file mode 100644
index 0000000000000..fa43f987e5667
--- /dev/null
+++ b/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll
@@ -0,0 +1,34 @@
+; Test to ensure that if a definition is imported, already-present declarations
+; are updated as necessary: Definitions from the same module may be optimized
+; together. Thus care must be taken when importing only a subset of the
+; definitions from a module (because other referenced definitions from that
+; module may have been changed by the optimizer and may no longer match
+; declarations already present in the module being imported into).
+
+; Generate bitcode and index, and run the function import.
+; `Inputs/attr_fixup_dae_noundef.ll` contains the post-"Dead Argument Elimination" IR, which
+; removed the `noundef` from `@inner`.
+; RUN: opt -module-summary %p/Inputs/attr_fixup_dae_noundef.ll -o %t.inputs.attr_fixup_dae_noundef.bc
+; RUN: opt -module-summary %s -o %t.main.bc
+; RUN: llvm-lto -thinlto -o %t.summary %t.main.bc %t.inputs.attr_fixup_dae_noundef.bc
+; RUN: opt -passes=function-import -summary-file %t.summary.thinlto.bc %t.main.bc -S 2>&1 \
+; RUN: | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @main() {
+ call void @outer(i32 noundef 1)
+ call void @inner(i32 noundef 1)
+ ret void
+}
+
+; Because `@inner` is `noinline`, it should not get imported. However, the
+; `noundef` should be removed.
+; CHECK: declare void @inner(i32)
+declare void @inner(i32 noundef)
+
+; `@outer` should get imported.
+; CHECK: define available_externally void @outer(i32 noundef %arg)
+; CHECK-NEXT: call void @inner(i32 poison)
+declare void @outer(i32 noundef)
More information about the llvm-commits
mailing list