[llvm] 86ced82 - Revert "[IRMover] Remove UB implying parameter attributes when necessary"
Arthur Eubanks via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 22 09:44:32 PST 2023
Author: Arthur Eubanks
Date: 2023-02-22T09:40:39-08:00
New Revision: 86ced82099813fa000e4681882115636240d76eb
URL: https://github.com/llvm/llvm-project/commit/86ced82099813fa000e4681882115636240d76eb
DIFF: https://github.com/llvm/llvm-project/commit/86ced82099813fa000e4681882115636240d76eb.diff
LOG: Revert "[IRMover] Remove UB implying parameter attributes when necessary"
This reverts commit 451799bb8261bde52bbfef226d019caf1d82aa42.
Causes crashes, see D139209
Added:
Modified:
llvm/include/llvm/IR/Attributes.h
llvm/lib/Linker/IRMover.cpp
Removed:
llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll
llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll
################################################################################
diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index fa179c9d367ea..c4e12a673ed23 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/llvm/include/llvm/IR/Attributes.h
@@ -360,13 +360,6 @@ 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 fc226302423cf..721acb1ea02ca 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 &DstGV, GlobalValue &SrcGV);
+ void updateAttributes(GlobalValue &GV);
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, *SGV);
+ updateAttributes(*New);
return New;
}
@@ -1533,10 +1533,7 @@ static std::string adjustInlineAsm(const std::string &InlineAsm,
return InlineAsm;
}
-void IRLinker::updateAttributes(GlobalValue &DstGV, GlobalValue &SrcGV) {
- auto *DstF = dyn_cast<Function>(&DstGV);
- auto *SrcF = dyn_cast<Function>(&SrcGV);
-
+void IRLinker::updateAttributes(GlobalValue &GV) {
/// 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
@@ -1551,53 +1548,16 @@ void IRLinker::updateAttributes(GlobalValue &DstGV, GlobalValue &SrcGV) {
/// 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 (DstF) {
- if (!DstF->isIntrinsic())
- DstF->removeFnAttr(llvm::Attribute::NoCallback);
+ if (auto *F = dyn_cast<Function>(&GV)) {
+ if (!F->isIntrinsic())
+ F->removeFnAttr(llvm::Attribute::NoCallback);
// Remove nocallback attribute when it is on a call-site.
- for (BasicBlock &BB : *DstF)
+ for (BasicBlock &BB : *F)
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
deleted file mode 100644
index c8e31217633d5..0000000000000
--- a/llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll
+++ /dev/null
@@ -1,17 +0,0 @@
-; 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
deleted file mode 100644
index fa43f987e5667..0000000000000
--- a/llvm/test/Transforms/FunctionImport/attr_fixup_dae_noundef.ll
+++ /dev/null
@@ -1,34 +0,0 @@
-; 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