[LLVM] Add InsertPosition union-type to remove overloads of Instruction-creation #94226
+411
−3,153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-adt @llvm/pr-subscribers-llvm-ir Author: Stephen Tozer (SLTozer) ChangesThis patch follows the previous PR #94224 and contains that patch's commit; to view the changes for only this patch, review only the second commit. This patch builds off the previous PR to simplify instruction creation, by replacing all overloads of instruction constructors/Create methods that are identical other than the The upshot of this is that code will be deduplicated, and all callsites will switch to calling the new unified version without any changes needed to make the compiler happy. There is at least one exception to this; the construction of The end result of this is performance-neutral, with no significant impact on the geomean performance (albeit with some specific projects seeing a compile time increase), and a slight reduction in clang build time and file size. Besides reducing line count heavily and simplifying code, this helps build towards the proposal to rewrite the instruction-insertion interface, which will help reduce future errors relating to incorrect instruction insertion in the presence of debug records. Getting this single large change in now will also make it much easier to deprecate the Patch is 265.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94226.diff 19 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuilder.h b/clang/lib/CodeGen/CGBuilder.h
index 6dd9da7c4cade..ed07476f4047f 100644
--- a/clang/lib/CodeGen/CGBuilder.h
+++ b/clang/lib/CodeGen/CGBuilder.h
@@ -35,7 +35,6 @@ class CGBuilderInserter final : public llvm::IRBuilderDefaultInserter {
/// This forwards to CodeGenFunction::InsertHelper.
void InsertHelper(llvm::Instruction *I, const llvm::Twine &Name,
- llvm::BasicBlock *BB,
llvm::BasicBlock::iterator InsertPt) const override;
private:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index d6478cc6835d8..4e3a96d442400 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -120,7 +120,8 @@ llvm::AllocaInst *CodeGenFunction::CreateTempAlloca(llvm::Type *Ty,
Alloca = Builder.CreateAlloca(Ty, ArraySize, Name);
else
Alloca = new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
- ArraySize, Name, AllocaInsertPt);
+ ArraySize, Name,
+ (llvm::Instruction *)AllocaInsertPt);
if (Allocas) {
Allocas->Add(Alloca);
}
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index f0345f3b191b8..bbec920b9868a 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2635,7 +2635,6 @@ CodeGenFunction::SanitizerScope::~SanitizerScope() {
void CodeGenFunction::InsertHelper(llvm::Instruction *I,
const llvm::Twine &Name,
- llvm::BasicBlock *BB,
llvm::BasicBlock::iterator InsertPt) const {
LoopStack.InsertHelper(I);
if (IsSanitizerScope)
@@ -2643,11 +2642,11 @@ void CodeGenFunction::InsertHelper(llvm::Instruction *I,
}
void CGBuilderInserter::InsertHelper(
- llvm::Instruction *I, const llvm::Twine &Name, llvm::BasicBlock *BB,
+ llvm::Instruction *I, const llvm::Twine &Name,
llvm::BasicBlock::iterator InsertPt) const {
- llvm::IRBuilderDefaultInserter::InsertHelper(I, Name, BB, InsertPt);
+ llvm::IRBuilderDefaultInserter::InsertHelper(I, Name, InsertPt);
if (CGF)
- CGF->InsertHelper(I, Name, BB, InsertPt);
+ CGF->InsertHelper(I, Name, InsertPt);
}
// Emits an error if we don't have a valid set of target features for the
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 45585361a4fc9..e3617395895ab 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -345,7 +345,6 @@ class CodeGenFunction : public CodeGenTypeCache {
/// CGBuilder insert helper. This function is called after an
/// instruction is created using Builder.
void InsertHelper(llvm::Instruction *I, const llvm::Twine &Name,
- llvm::BasicBlock *BB,
llvm::BasicBlock::iterator InsertPt) const;
/// CurFuncDecl - Holds the Decl for the current outermost
diff --git a/llvm/include/llvm/ADT/ilist_base.h b/llvm/include/llvm/ADT/ilist_base.h
index b8c098b951ade..000253a26012b 100644
--- a/llvm/include/llvm/ADT/ilist_base.h
+++ b/llvm/include/llvm/ADT/ilist_base.h
@@ -15,9 +15,9 @@
namespace llvm {
/// Implementations of list algorithms using ilist_node_base.
-template <bool EnableSentinelTracking> class ilist_base {
+template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_base {
public:
- using node_base_type = ilist_node_base<EnableSentinelTracking>;
+ using node_base_type = ilist_node_base<EnableSentinelTracking, ParentPtrTy>;
static void insertBeforeImpl(node_base_type &Next, node_base_type &N) {
node_base_type &Prev = *Next.getPrev();
diff --git a/llvm/include/llvm/ADT/ilist_iterator.h b/llvm/include/llvm/ADT/ilist_iterator.h
index 2393c4d2c403c..635e050e0d09a 100644
--- a/llvm/include/llvm/ADT/ilist_iterator.h
+++ b/llvm/include/llvm/ADT/ilist_iterator.h
@@ -70,6 +70,7 @@ class ilist_iterator : ilist_detail::SpecificNodeAccess<OptionsT> {
using iterator_category = std::bidirectional_iterator_tag;
using const_pointer = typename OptionsT::const_pointer;
using const_reference = typename OptionsT::const_reference;
+ using parent_ptr_ty = typename OptionsT::parent_ptr_ty;
private:
using node_pointer = typename Traits::node_pointer;
@@ -101,6 +102,8 @@ class ilist_iterator : ilist_detail::SpecificNodeAccess<OptionsT> {
return *this;
}
+ parent_ptr_ty getNodeParent() { return NodePtr->getNodeBaseParent(); }
+
/// Explicit conversion between forward/reverse iterators.
///
/// Translate between forward and reverse iterators without changing range
@@ -168,6 +171,8 @@ class ilist_iterator : ilist_detail::SpecificNodeAccess<OptionsT> {
return tmp;
}
+ bool isValid() const { return NodePtr; }
+
/// Get the underlying ilist_node.
node_pointer getNodePtr() const { return static_cast<node_pointer>(NodePtr); }
@@ -195,6 +200,7 @@ class ilist_iterator_w_bits : ilist_detail::SpecificNodeAccess<OptionsT> {
using iterator_category = std::bidirectional_iterator_tag;
using const_pointer = typename OptionsT::const_pointer;
using const_reference = typename OptionsT::const_reference;
+ using parent_ptr_ty = typename OptionsT::parent_ptr_ty;
private:
using node_pointer = typename Traits::node_pointer;
@@ -319,6 +325,10 @@ class ilist_iterator_w_bits : ilist_detail::SpecificNodeAccess<OptionsT> {
return tmp;
}
+ parent_ptr_ty getNodeParent() { return NodePtr->getNodeBaseParent(); }
+
+ bool isValid() const { return NodePtr; }
+
/// Get the underlying ilist_node.
node_pointer getNodePtr() const { return static_cast<node_pointer>(NodePtr); }
diff --git a/llvm/include/llvm/ADT/ilist_node.h b/llvm/include/llvm/ADT/ilist_node.h
index 3b6f0dcc7b5e9..ec615497abee1 100644
--- a/llvm/include/llvm/ADT/ilist_node.h
+++ b/llvm/include/llvm/ADT/ilist_node.h
@@ -118,7 +118,9 @@ template <class OptionsT> class ilist_node_impl : OptionsT::node_base_type {
}
// Under-approximation, but always available for assertions.
+ using node_base_type::getNodeBaseParent;
using node_base_type::isKnownSentinel;
+ using node_base_type::setNodeBaseParent;
/// Check whether this is the sentinel node.
///
@@ -171,6 +173,15 @@ template <class OptionsT> class ilist_node_impl : OptionsT::node_base_type {
/// }
/// \endexample
///
+/// When the \a ilist_parent<ParentTy> option is passed to an ilist_node and the
+/// owning ilist, each node contains a pointer to the ilist's owner. This
+/// pointer does not have any automatic behaviour; set it manually, including
+/// for the sentinel node when the list is created. The primary benefit of this
+/// over declaring and using this pointer in the final node class is that the
+/// pointer will be added in the sentinel, meaning that you can safely use \a
+/// ilist_iterator::getNodeParent() to get the node parent from any valid (i.e.
+/// non-null) iterator, even a sentinel value.
+///
/// See \a is_valid_option for steps on adding a new option.
template <class T, class... Options>
class ilist_node
diff --git a/llvm/include/llvm/ADT/ilist_node_base.h b/llvm/include/llvm/ADT/ilist_node_base.h
index f6c518e6eed7a..e396bb22fca02 100644
--- a/llvm/include/llvm/ADT/ilist_node_base.h
+++ b/llvm/include/llvm/ADT/ilist_node_base.h
@@ -16,9 +16,9 @@ namespace llvm {
/// Base class for ilist nodes.
///
/// Optionally tracks whether this node is the sentinel.
-template <bool EnableSentinelTracking> class ilist_node_base;
+template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_node_base;
-template <> class ilist_node_base<false> {
+template <> class ilist_node_base<false, void> {
ilist_node_base *Prev = nullptr;
ilist_node_base *Next = nullptr;
@@ -28,11 +28,14 @@ template <> class ilist_node_base<false> {
ilist_node_base *getPrev() const { return Prev; }
ilist_node_base *getNext() const { return Next; }
+ void setNodeBaseParent(void) {}
+ inline void getNodeBaseParent() const {}
+
bool isKnownSentinel() const { return false; }
void initializeSentinel() {}
};
-template <> class ilist_node_base<true> {
+template <> class ilist_node_base<true, void> {
PointerIntPair<ilist_node_base *, 1> PrevAndSentinel;
ilist_node_base *Next = nullptr;
@@ -42,6 +45,48 @@ template <> class ilist_node_base<true> {
ilist_node_base *getPrev() const { return PrevAndSentinel.getPointer(); }
ilist_node_base *getNext() const { return Next; }
+ void setNodeBaseParent(void) {}
+ inline void getNodeBaseParent() const {}
+
+ bool isSentinel() const { return PrevAndSentinel.getInt(); }
+ bool isKnownSentinel() const { return isSentinel(); }
+ void initializeSentinel() { PrevAndSentinel.setInt(true); }
+};
+
+template <class ParentPtrTy> class ilist_node_base<false, ParentPtrTy> {
+ ilist_node_base *Prev = nullptr;
+ ilist_node_base *Next = nullptr;
+ ParentPtrTy Parent = nullptr;
+
+public:
+ void setPrev(ilist_node_base *Prev) { this->Prev = Prev; }
+ void setNext(ilist_node_base *Next) { this->Next = Next; }
+ ilist_node_base *getPrev() const { return Prev; }
+ ilist_node_base *getNext() const { return Next; }
+
+ void setNodeBaseParent(ParentPtrTy Parent) { this->Parent = Parent; }
+ inline const ParentPtrTy getNodeBaseParent() const { return Parent; }
+ inline ParentPtrTy getNodeBaseParent() { return Parent; }
+
+ bool isKnownSentinel() const { return false; }
+ void initializeSentinel() {}
+};
+
+template <class ParentPtrTy> class ilist_node_base<true, ParentPtrTy> {
+ PointerIntPair<ilist_node_base *, 1> PrevAndSentinel;
+ ilist_node_base *Next = nullptr;
+ ParentPtrTy Parent = nullptr;
+
+public:
+ void setPrev(ilist_node_base *Prev) { PrevAndSentinel.setPointer(Prev); }
+ void setNext(ilist_node_base *Next) { this->Next = Next; }
+ ilist_node_base *getPrev() const { return PrevAndSentinel.getPointer(); }
+ ilist_node_base *getNext() const { return Next; }
+
+ void setNodeBaseParent(ParentPtrTy Parent) { this->Parent = Parent; }
+ inline const ParentPtrTy getNodeBaseParent() const { return Parent; }
+ inline ParentPtrTy getNodeBaseParent() { return Parent; }
+
bool isSentinel() const { return PrevAndSentinel.getInt(); }
bool isKnownSentinel() const { return isSentinel(); }
void initializeSentinel() { PrevAndSentinel.setInt(true); }
diff --git a/llvm/include/llvm/ADT/ilist_node_options.h b/llvm/include/llvm/ADT/ilist_node_options.h
index e6e1068953e36..aa32162cd51e4 100644
--- a/llvm/include/llvm/ADT/ilist_node_options.h
+++ b/llvm/include/llvm/ADT/ilist_node_options.h
@@ -15,8 +15,8 @@
namespace llvm {
-template <bool EnableSentinelTracking> class ilist_node_base;
-template <bool EnableSentinelTracking> class ilist_base;
+template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_node_base;
+template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_base;
/// Option to choose whether to track sentinels.
///
@@ -39,6 +39,19 @@ template <class Tag> struct ilist_tag {};
/// iterator class to store that information.
template <bool ExtraIteratorBits> struct ilist_iterator_bits {};
+/// Option to add a pointer to this list's owner in every node.
+///
+/// This option causes the \a ilist_base_node for this list to contain a pointer
+/// ParentTy *Parent, returned by \a ilist_base_node::getNodeBaseParent() and
+/// set by \a ilist_base_node::setNodeBaseParent(ParentTy *Parent). The parent
+/// value is not set automatically; the ilist owner should set itself as the
+/// parent of the list sentinel, and the parent should be set on each node
+/// inserted into the list. This value is also not used by
+/// \a ilist_node_with_parent::getNodeParent(), but is used by \a
+/// ilist_iterator::getNodeParent(), which allows the parent to be fetched from
+/// any valid (non-null) iterator to this list, including the sentinel.
+template <class ParentTy> struct ilist_parent {};
+
namespace ilist_detail {
/// Helper trait for recording whether an option is specified explicitly.
@@ -114,6 +127,21 @@ template <> struct extract_iterator_bits<> : std::false_type, is_implicit {};
template <bool IteratorBits>
struct is_valid_option<ilist_iterator_bits<IteratorBits>> : std::true_type {};
+/// Extract node parent option.
+///
+/// Look through \p Options for the \a ilist_parent option, pulling out the
+/// custom parent type, using void as a default.
+template <class... Options> struct extract_parent;
+template <class ParentTy, class... Options>
+struct extract_parent<ilist_parent<ParentTy>, Options...> {
+ typedef ParentTy *type;
+};
+template <class Option1, class... Options>
+struct extract_parent<Option1, Options...> : extract_parent<Options...> {};
+template <> struct extract_parent<> { typedef void type; };
+template <class ParentTy>
+struct is_valid_option<ilist_parent<ParentTy>> : std::true_type {};
+
/// Check whether options are valid.
///
/// The conjunction of \a is_valid_option on each individual option.
@@ -128,7 +156,7 @@ struct check_options<Option1, Options...>
///
/// This is usually computed via \a compute_node_options.
template <class T, bool EnableSentinelTracking, bool IsSentinelTrackingExplicit,
- class TagT, bool HasIteratorBits>
+ class TagT, bool HasIteratorBits, class ParentPtrTy>
struct node_options {
typedef T value_type;
typedef T *pointer;
@@ -140,15 +168,18 @@ struct node_options {
static const bool is_sentinel_tracking_explicit = IsSentinelTrackingExplicit;
static const bool has_iterator_bits = HasIteratorBits;
typedef TagT tag;
- typedef ilist_node_base<enable_sentinel_tracking> node_base_type;
- typedef ilist_base<enable_sentinel_tracking> list_base_type;
+ typedef ParentPtrTy parent_ptr_ty;
+ typedef ilist_node_base<enable_sentinel_tracking, parent_ptr_ty>
+ node_base_type;
+ typedef ilist_base<enable_sentinel_tracking, parent_ptr_ty> list_base_type;
};
template <class T, class... Options> struct compute_node_options {
typedef node_options<T, extract_sentinel_tracking<Options...>::value,
extract_sentinel_tracking<Options...>::is_explicit,
typename extract_tag<Options...>::type,
- extract_iterator_bits<Options...>::value>
+ extract_iterator_bits<Options...>::value,
+ typename extract_parent<Options...>::type>
type;
};
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index e1220966e7e6e..80067f2652a2b 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -59,7 +59,8 @@ class DbgMarker;
class BasicBlock final : public Value, // Basic blocks are data objects also
public ilist_node_with_parent<BasicBlock, Function> {
public:
- using InstListType = SymbolTableList<Instruction, ilist_iterator_bits<true>>;
+ using InstListType = SymbolTableList<Instruction, ilist_iterator_bits<true>,
+ ilist_parent<BasicBlock>>;
/// Flag recording whether or not this block stores debug-info in the form
/// of intrinsic instructions (false) or non-instruction records (true).
bool IsNewDbgInfoFormat;
@@ -172,10 +173,11 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
friend BasicBlock::iterator Instruction::eraseFromParent();
friend BasicBlock::iterator Instruction::insertInto(BasicBlock *BB,
BasicBlock::iterator It);
- friend class llvm::SymbolTableListTraits<llvm::Instruction,
- ilist_iterator_bits<true>>;
+ friend class llvm::SymbolTableListTraits<
+ llvm::Instruction, ilist_iterator_bits<true>, ilist_parent<BasicBlock>>;
friend class llvm::ilist_node_with_parent<llvm::Instruction, llvm::BasicBlock,
- ilist_iterator_bits<true>>;
+ ilist_iterator_bits<true>,
+ ilist_parent<BasicBlock>>;
// Friendly methods that need to access us for the maintenence of
// debug-info attachments.
diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h
index 40a9cf507248a..9a9299960a121 100644
--- a/llvm/include/llvm/IR/IRBuilder.h
+++ b/llvm/include/llvm/IR/IRBuilder.h
@@ -63,10 +63,10 @@ class IRBuilderDefaultInserter {
virtual ~IRBuilderDefaultInserter();
virtual void InsertHelper(Instruction *I, const Twine &Name,
- BasicBlock *BB,
BasicBlock::iterator InsertPt) const {
- if (BB)
- I->insertInto(BB, InsertPt);
+ if (InsertPt.isValid()) {
+ I->insertInto(InsertPt.getNodeParent(), InsertPt);
+ }
I->setName(Name);
}
};
@@ -83,9 +83,8 @@ class IRBuilderCallbackInserter : public IRBuilderDefaultInserter {
: Callback(std::move(Callback)) {}
void InsertHelper(Instruction *I, const Twine &Name,
- BasicBlock *BB,
BasicBlock::iterator InsertPt) const override {
- IRBuilderDefaultInserter::InsertHelper(I, Name, BB, InsertPt);
+ IRBuilderDefaultInserter::InsertHelper(I, Name, InsertPt);
Callback(I);
}
};
@@ -143,7 +142,7 @@ class IRBuilderBase {
/// Insert and return the specified instruction.
template<typename InstTy>
InstTy *Insert(InstTy *I, const Twine &Name = "") const {
- Inserter.InsertHelper(I, Name, BB, InsertPt);
+ Inserter.InsertHelper(I, Name, InsertPt);
AddMetadataToInst(I);
return I;
}
diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
index 9dd1bb455a718..2ff1c7f9ed760 100644
--- a/llvm/include/llvm/IR/InstrTypes.h
+++ b/llvm/include/llvm/IR/InstrTypes.h
@@ -107,12 +107,8 @@ class UnaryOperator : public UnaryInstruction {
void AssertOK();
protected:
- UnaryOperator(UnaryOps iType, Value *S, Type *Ty,
- const Twine &Name, BasicBlock::iterator InsertBefore);
- UnaryOperator(UnaryOps iType, Value *S, Type *Ty,
- const Twine &Name, Instruction *InsertBefore);
- UnaryOperator(UnaryOps iType, Value *S, Type *Ty,
- const Twine &Name, BasicBlock *InsertAtEnd);
+ UnaryOperator(UnaryOps iType, Value *S, Type *Ty, const Twine &Name,
+ InsertPosition InsertBefore = nullptr);
// Note: Instruction needs to be a friend here to call cloneImpl.
friend class Instruction;
@@ -120,13 +116,6 @@ class UnaryOperator : public UnaryInstruction {
UnaryOperator *cloneImpl() const;
public:
- /// Construct a unary instruction, given the opcode and an operand.
- /// Insert the instruction into a BasicBlock right before the specified
- /// instruction (InsertBefore must be a valid iterator).
- ///
- static UnaryOperator *Create(UnaryOps Op, Value *S, const Twine &Name,
- BasicBlock::iterator InsertBefore);
-
/// Construct a unary instruction, given the opcode and an operand.
/// Optionally (if InstBefore is specified) insert the instruction
/// into a BasicBlock right before the specified instruction. The specified
@@ -134,15 +123,7 @@ class UnaryOperator : public UnaryInstruction {
///
static UnaryOperator *Create(UnaryOps Op, Value *S,
const Twine &Name = Twine(),
- Instruction *InsertBefore = nullptr);
-
- /// Construct a unary instruction, given the opcode and an operand.
- /// Also automatically insert this instruction to the end of the
- /// BasicBlock specified.
- ///
- static UnaryOperator *Create(UnaryOps Op, Value *S,
- const Twine &Name,
- BasicBlock *InsertAtEnd);
+ InsertPosition InsertBefore = nullptr);
/// These methods just forward to Create, and are useful when you
/// statically know what type of instruction you're going to create. These
@@ -171,33 +152,18 @@ class UnaryOperator : public UnaryInstruction {
}
#include "llvm/IR/Instruction.def"
- static UnaryOperator *
- CreateWithCopiedFlags(UnaryOps Opc, Value *V, Instruction *CopyO,
- const Twine &Name, BasicBlock::iterator InsertBe...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SLTozer
force-pushed
the
add-instruction-insert-position
branch
from
ffd3172 to
da18a11
Compare
nikic
approved these changes
Jun 19, 2024
| if (BB) | ||
| I->insertInto(BB, InsertPt); | ||
| if (InsertPt.isValid()) { | ||
| I->insertInto(InsertPt.getNodeParent(), InsertPt); |
There was a problem hiding this comment.
We should drop the BasicBlock argument from this API now, but that can be a followup...
|
It looks like polly needs an update as well. |
nikic
reviewed
Jun 19, 2024
|
Alright, build's now working for LLVM, Clang, Flang, Polly, and MLIR; looks good to merge? |
nikic
approved these changes
Jun 19, 2024
|
I think some of the overloads for constructing an instruction aren't quite right: in some cases, we require a non-null insertion-point so we can retrieve the DataLayout from the insertion-point. (Particularly instructions that have an explicitly specified alignment.) In those cases, we shouldn't default to "= nullptr": a null insertion point is guaranteed to assert at runtime. |
That's reasonable - I defaulted to adding nullptr-defaults in the case where it didn't cause incorrect overloads, but it's easier to keep only the existing defaults and let any others be added later as appropriate; I've updated the patch accordingly. |
efriedma-quic
approved these changes
Jun 19, 2024
SLTozer
added a commit
that referenced
this pull request
Jun 24, 2024
Uses the new InsertPosition class (added in #94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the `Instruction *InsertBefore` argument from instruction-creation (discussed [here](https://discourse.llvm.org/t/psa-instruction-constructors-changing-to-iterator-only-insertion/77845)), this will simplify the process by allowing us to deprecate the InsertPosition constructor directly and catch all the cases where we use instructions rather than iterators.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
…on-creation (llvm#94226) This patch simplifies instruction creation by replacing all overloads of instruction constructors/Create methods that are identical other than the Instruction *InsertBefore/BasicBlock *InsertAtEnd/BasicBlock::iterator InsertBefore argument with a single version that takes an InsertPosition argument. The InsertPosition class can be implicitly constructed from any of the above, internally converting them to the appropriate BasicBlock::iterator value which can then be used to insert the instruction (or to not insert it if an invalid iterator is passed). The upshot of this is that code will be deduplicated, and all callsites will switch to calling the new unified version without any changes needed to make the compiler happy. There is at least one exception to this; the construction of InsertPosition is a user-defined conversion, so any caller that was already relying on a different user-defined conversion won't work. In all of LLVM and Clang this happens exactly once: at clang/lib/CodeGen/CGExpr.cpp:123 we try to construct an alloca with an AssertingVH<Instruction> argument, which must now be cast to an Instruction* by using `&*`. If this is more common elsewhere, it could be fixed by adding an appropriate constructor to InsertPosition.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
Uses the new InsertPosition class (added in llvm#94226) to simplify some of the IRBuilder interface, and removes the need to pass a BasicBlock alongside a BasicBlock::iterator, using the fact that we can now get the parent basic block from the iterator even if it points to the sentinel. This patch removes the BasicBlock argument from each constructor or call to setInsertPoint. This has no functional effect, but later on as we look to remove the `Instruction *InsertBefore` argument from instruction-creation (discussed [here](https://discourse.llvm.org/t/psa-instruction-constructors-changing-to-iterator-only-insertion/77845)), this will simplify the process by allowing us to deprecate the InsertPosition constructor directly and catch all the cases where we use instructions rather than iterators.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch follows the previous PR #94224 and contains that patch's commit; to view the changes for only this patch, review only the second commit.
This patch builds off the previous PR to simplify instruction creation, by replacing all overloads of instruction constructors/Create methods that are identical other than the
Instruction *InsertBefore/BasicBlock *InsertAtEnd/BasicBlock::iterator InsertBeforeargument with a single version that takes anInsertPositionargument. TheInsertPositionclass can be implicitly constructed from any of the above, internally converting them to the appropriateBasicBlock::iteratorvalue which can then be used to insert the instruction (or to not insert it if an invalid iterator is passed).The upshot of this is that code will be deduplicated, and all callsites will switch to calling the new unified version without any changes needed to make the compiler happy. There is at least one exception to this; the construction of
InsertPositionis a user-defined conversion, so any caller that was already relying on a different user-defined conversion won't work. In all of LLVM and Clang this happens exactly once: atclang/lib/CodeGen/CGExpr.cpp:123we try to construct an alloca with anAssertingVH<Instruction>argument, which must now be explicitly cast to anInstruction*. If this is more common elsewhere, it could be fixed by adding an appropriate constructor toInsertPosition.The end result of this is performance-neutral, with no significant impact on the geomean performance (albeit with some specific projects seeing a compile time increase), and a slight reduction in clang build time and file size. Besides reducing line count heavily and simplifying code, this helps build towards the proposal to rewrite the instruction-insertion interface, which will help reduce future errors relating to incorrect instruction insertion in the presence of debug records. Getting this single large change in now will also make it much easier to deprecate the
Instruction*insertion arguments, as proposed on discourse, as we can do so with a singleLLVM_DEPRECATEDtag rather than needing to update every single method.