Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build failure caused by PR 68235 #71219

Closed
wants to merge 2 commits into from

Conversation

manman-ren
Copy link
Contributor

We move the implementations to Transforms/Utils:
isEligibleInstrunctionForConstantSharing
isEligibleOperandForConstantSharing
as they are used from Transforms/Utils/FunctionComparatorIgnoringConst.cpp.

We move the implementations to Transforms/Utils:
isEligibleInstrunctionForConstantSharing
isEligibleOperandForConstantSharing
as they are used from Transforms/Utils/FunctionComparatorIgnoringConst.cpp.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 3, 2023

@llvm/pr-subscribers-llvm-transforms

Author: Manman Ren (manman-ren)

Changes

We move the implementations to Transforms/Utils:
isEligibleInstrunctionForConstantSharing
isEligibleOperandForConstantSharing
as they are used from Transforms/Utils/FunctionComparatorIgnoringConst.cpp.


Full diff: https://github.com/llvm/llvm-project/pull/71219.diff

3 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/MergeFunctionsIgnoringConst.cpp (+2-72)
  • (modified) llvm/lib/Transforms/Utils/FunctionComparatorIgnoringConst.cpp (+62)
  • (modified) llvm/test/Transforms/MergeFuncIgnoringConst/merge_with_exception.ll (+1-1)
diff --git a/llvm/lib/Transforms/IPO/MergeFunctionsIgnoringConst.cpp b/llvm/lib/Transforms/IPO/MergeFunctionsIgnoringConst.cpp
index d6ae788ddb9e1a1..225709b8b3caf69 100644
--- a/llvm/lib/Transforms/IPO/MergeFunctionsIgnoringConst.cpp
+++ b/llvm/lib/Transforms/IPO/MergeFunctionsIgnoringConst.cpp
@@ -50,6 +50,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/Utils/FunctionComparatorIgnoringConst.h"
+#include "llvm/Transforms/Utils/MergeFunctionsIgnoringConst.h"
 #include <vector>
 
 using namespace llvm;
@@ -112,64 +113,11 @@ static cl::list<std::string> MergeAllowRegexFilters(
              "regular expression"),
     cl::ZeroOrMore);
 
-bool isEligibleInstrunctionForConstantSharing(const Instruction *I) {
-  switch (I->getOpcode()) {
-  case Instruction::Load:
-  case Instruction::Store:
-  case Instruction::Call:
-    return true;
-  default: {
-    if (EnableAggressiveMergeFunc && I->getOpcode() == Instruction::Invoke)
-      return true;
-    return false;
-  }
-  }
-}
-
 /// Returns true if the \OpIdx operand of \p CI is the callee operand.
 static bool isCalleeOperand(const CallBase *CI, unsigned OpIdx) {
   return &CI->getCalledOperandUse() == &CI->getOperandUse(OpIdx);
 }
 
-static bool canParameterizeCallOperand(const CallBase *CI, unsigned OpIdx) {
-  if (CI->isInlineAsm())
-    return false;
-  Function *Callee = CI->getCalledOperand()
-                         ? dyn_cast_or_null<Function>(
-                               CI->getCalledOperand()->stripPointerCasts())
-                         : nullptr;
-  if (Callee) {
-    if (Callee->isIntrinsic())
-      return false;
-    // objc_msgSend stubs must be called, and can't have their address taken.
-    if (Callee->getName().startswith("objc_msgSend$"))
-      return false;
-  }
-  if (isCalleeOperand(CI, OpIdx) &&
-      CI->getOperandBundle(LLVMContext::OB_ptrauth).has_value()) {
-    // The operand is the callee and it has already been signed. Ignore this
-    // because we cannot add another ptrauth bundle to the call instruction.
-    return false;
-  }
-  return true;
-}
-
-bool isEligibleOperandForConstantSharing(const Instruction *I, unsigned OpIdx) {
-  assert(OpIdx < I->getNumOperands() && "Invalid operand index");
-
-  if (!isEligibleInstrunctionForConstantSharing(I))
-    return false;
-
-  auto Opnd = I->getOperand(OpIdx);
-  if (!isa<Constant>(Opnd))
-    return false;
-
-  if (const auto *CI = dyn_cast<CallBase>(I))
-    return canParameterizeCallOperand(CI, OpIdx);
-
-  return true;
-}
-
 namespace {
 
 /// MergeFuncIgnoringConst finds functions which only differ by constants in
@@ -1268,25 +1216,7 @@ static llvm::AttributeList
 fixUpTypesInByValAndStructRetAttributes(llvm::FunctionType *fnType,
                                         llvm::AttributeList attrList) {
   auto &context = fnType->getContext();
-  if (!context.supportsTypedPointers())
-    return attrList;
-
-  for (unsigned i = 0; i < fnType->getNumParams(); ++i) {
-    auto paramTy = fnType->getParamType(i);
-    auto attrListIndex = llvm::AttributeList::FirstArgIndex + i;
-    if (attrList.hasParamAttr(i, llvm::Attribute::StructRet) &&
-        paramTy->getNonOpaquePointerElementType() !=
-            attrList.getParamStructRetType(i))
-      attrList = attrList.replaceAttributeTypeAtIndex(
-          context, attrListIndex, llvm::Attribute::StructRet,
-          paramTy->getNonOpaquePointerElementType());
-    if (attrList.hasParamAttr(i, llvm::Attribute::ByVal) &&
-        paramTy->getNonOpaquePointerElementType() !=
-            attrList.getParamByValType(i))
-      attrList = attrList.replaceAttributeTypeAtIndex(
-          context, attrListIndex, llvm::Attribute::ByVal,
-          paramTy->getNonOpaquePointerElementType());
-  }
+  // supportsTypedPointers always returns false now.
   return attrList;
 }
 
diff --git a/llvm/lib/Transforms/Utils/FunctionComparatorIgnoringConst.cpp b/llvm/lib/Transforms/Utils/FunctionComparatorIgnoringConst.cpp
index 9cfd95345598083..ab0de3659c92a7c 100644
--- a/llvm/lib/Transforms/Utils/FunctionComparatorIgnoringConst.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionComparatorIgnoringConst.cpp
@@ -10,9 +10,71 @@
 
 #include "llvm/Transforms/Utils/FunctionComparatorIgnoringConst.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Transforms/Utils/MergeFunctionsIgnoringConst.h"
 
 using namespace llvm;
+static cl::opt<bool> EnableMergeOnInvoke(
+    "enable-aggressive-mergefunc-invoke", cl::init(false), cl::Hidden,
+    cl::desc("Enable aggressive function merger on Invoke"));
+
+/// Returns true if the \OpIdx operand of \p CI is the callee operand.
+static bool isCalleeOperand(const CallBase *CI, unsigned OpIdx) {
+  return &CI->getCalledOperandUse() == &CI->getOperandUse(OpIdx);
+}
+
+bool isEligibleInstrunctionForConstantSharing(const Instruction *I) {
+  switch (I->getOpcode()) {
+  case Instruction::Load:
+  case Instruction::Store:
+  case Instruction::Call:
+    return true;
+  default: {
+    if (EnableMergeOnInvoke && I->getOpcode() == Instruction::Invoke)
+      return true;
+    return false;
+  }
+  }
+}
+
+static bool canParameterizeCallOperand(const CallBase *CI, unsigned OpIdx) {
+  if (CI->isInlineAsm())
+    return false;
+  Function *Callee = CI->getCalledOperand()
+                         ? dyn_cast_or_null<Function>(
+                               CI->getCalledOperand()->stripPointerCasts())
+                         : nullptr;
+  if (Callee) {
+    if (Callee->isIntrinsic())
+      return false;
+    // objc_msgSend stubs must be called, and can't have their address taken.
+    if (Callee->getName().startswith("objc_msgSend$"))
+      return false;
+  }
+  if (isCalleeOperand(CI, OpIdx) &&
+      CI->getOperandBundle(LLVMContext::OB_ptrauth).has_value()) {
+    // The operand is the callee and it has already been signed. Ignore this
+    // because we cannot add another ptrauth bundle to the call instruction.
+    return false;
+  }
+  return true;
+}
+
+bool isEligibleOperandForConstantSharing(const Instruction *I, unsigned OpIdx) {
+  assert(OpIdx < I->getNumOperands() && "Invalid operand index");
+
+  if (!isEligibleInstrunctionForConstantSharing(I))
+    return false;
+
+  auto Opnd = I->getOperand(OpIdx);
+  if (!isa<Constant>(Opnd))
+    return false;
+
+  if (const auto *CI = dyn_cast<CallBase>(I))
+    return canParameterizeCallOperand(CI, OpIdx);
+
+  return true;
+}
 
 int FunctionComparatorIgnoringConst::cmpOperandsIgnoringConsts(
     const Instruction *L, const Instruction *R, unsigned opIdx) {
diff --git a/llvm/test/Transforms/MergeFuncIgnoringConst/merge_with_exception.ll b/llvm/test/Transforms/MergeFuncIgnoringConst/merge_with_exception.ll
index c5c8b898c046e51..6544cafc8880d6b 100644
--- a/llvm/test/Transforms/MergeFuncIgnoringConst/merge_with_exception.ll
+++ b/llvm/test/Transforms/MergeFuncIgnoringConst/merge_with_exception.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -enable-aggressive-mergefunc-ignoringconst -passes=mergefunc-ignoring-const %s -o - | FileCheck %s
+; RUN: opt -S -enable-aggressive-mergefunc-ignoringconst -enable-aggressive-mergefunc-invoke -passes=mergefunc-ignoring-const %s -o - | FileCheck %s
 
 %4 = type opaque
 %10 = type opaque

Copy link
Contributor

@ellishg ellishg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepting to unblock the builds as there seems to be no functional change

@manman-ren
Copy link
Contributor Author

Fix build breakage: #68235

@@ -1268,25 +1216,7 @@ static llvm::AttributeList
fixUpTypesInByValAndStructRetAttributes(llvm::FunctionType *fnType,
llvm::AttributeList attrList) {
auto &context = fnType->getContext();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop context?

context, attrListIndex, llvm::Attribute::ByVal,
paramTy->getNonOpaquePointerElementType());
}
// supportsTypedPointers always returns false now.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe start with TODO:?

@vitalybuka vitalybuka changed the title Fix build failure caused by PR 68235 Fix build failure caused by #68235 Nov 3, 2023
@vitalybuka vitalybuka changed the title Fix build failure caused by #68235 Fix build failure caused by PR 68235 Nov 3, 2023
@RKSimon
Copy link
Collaborator

RKSimon commented Apr 5, 2024

@manman-ren OK to close this?

@manman-ren manman-ren closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants