[IR] Make instr-create fns with Instruction *InsertBefore non-default #86132
Conversation
|
@llvm/pr-subscribers-llvm-ir Author: Stephen Tozer (SLTozer) ChangesThis patch changes each of the methods that create instructions and may insert it at the same time - generally Unclear who's best placed to review this, but since this patch should be NFC - even for downstream consumers - it's relatively low-stakes; for the subsequent patch that deprecates the Instruction versions, I'll put out a RFC on discourse. Patch is 98.24 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86132.diff 4 Files Affected:
diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
index e8c2cba8418dc8..4d5ccbbbb46f3d 100644
--- a/llvm/include/llvm/IR/InstrTypes.h
+++ b/llvm/include/llvm/IR/InstrTypes.h
@@ -60,11 +60,11 @@ class UnaryInstruction : public Instruction {
Op<0>() = V;
}
UnaryInstruction(Type *Ty, unsigned iType, Value *V,
- Instruction *IB = nullptr)
+ Instruction *IB)
: Instruction(Ty, iType, &Op<0>(), 1, IB) {
Op<0>() = V;
}
- UnaryInstruction(Type *Ty, unsigned iType, Value *V, BasicBlock *IAE)
+ UnaryInstruction(Type *Ty, unsigned iType, Value *V, BasicBlock *IAE = nullptr)
: Instruction(Ty, iType, &Op<0>(), 1, IAE) {
Op<0>() = V;
}
@@ -132,16 +132,16 @@ class UnaryOperator : public UnaryInstruction {
/// Instruction is allowed to be a dereferenced end iterator.
///
static UnaryOperator *Create(UnaryOps Op, Value *S,
- const Twine &Name = Twine(),
- Instruction *InsertBefore = nullptr);
+ const Twine &Name,
+ Instruction *InsertBefore);
/// 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);
+ const Twine &Name = Twine(),
+ BasicBlock *InsertAtEnd = nullptr);
/// These methods just forward to Create, and are useful when you
/// statically know what type of instruction you're going to create. These
@@ -180,13 +180,22 @@ class UnaryOperator : public UnaryInstruction {
static UnaryOperator *
CreateWithCopiedFlags(UnaryOps Opc, Value *V, Instruction *CopyO,
- const Twine &Name = "",
- Instruction *InsertBefore = nullptr) {
+ const Twine &Name,
+ Instruction *InsertBefore) {
UnaryOperator *UO = Create(Opc, V, Name, InsertBefore);
UO->copyIRFlags(CopyO);
return UO;
}
+ static UnaryOperator *
+ CreateWithCopiedFlags(UnaryOps Opc, Value *V, Instruction *CopyO,
+ const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr) {
+ UnaryOperator *UO = Create(Opc, V, Name, InsertAtEnd);
+ UO->copyIRFlags(CopyO);
+ return UO;
+ }
+
static UnaryOperator *CreateFNegFMF(Value *Op, Instruction *FMFSource,
const Twine &Name,
BasicBlock::iterator InsertBefore) {
@@ -195,12 +204,19 @@ class UnaryOperator : public UnaryInstruction {
}
static UnaryOperator *CreateFNegFMF(Value *Op, Instruction *FMFSource,
- const Twine &Name = "",
- Instruction *InsertBefore = nullptr) {
+ const Twine &Name,
+ Instruction *InsertBefore) {
return CreateWithCopiedFlags(Instruction::FNeg, Op, FMFSource, Name,
InsertBefore);
}
+ static UnaryOperator *CreateFNegFMF(Value *Op, Instruction *FMFSource,
+ const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr) {
+ return CreateWithCopiedFlags(Instruction::FNeg, Op, FMFSource, Name,
+ InsertAtEnd);
+ }
+
UnaryOps getOpcode() const {
return static_cast<UnaryOps>(Instruction::getOpcode());
}
@@ -256,15 +272,15 @@ class BinaryOperator : public Instruction {
/// Instruction is allowed to be a dereferenced end iterator.
///
static BinaryOperator *Create(BinaryOps Op, Value *S1, Value *S2,
- const Twine &Name = Twine(),
- Instruction *InsertBefore = nullptr);
+ const Twine &Name,
+ Instruction *InsertBefore);
/// Construct a binary instruction, given the opcode and the two
/// operands. Also automatically insert this instruction to the end of the
/// BasicBlock specified.
///
static BinaryOperator *Create(BinaryOps Op, Value *S1, Value *S2,
- const Twine &Name, BasicBlock *InsertAtEnd);
+ const Twine &Name = Twine(), BasicBlock *InsertAtEnd = nullptr);
/// These methods just forward to Create, and are useful when you
/// statically know what type of instruction you're going to create. These
@@ -304,13 +320,22 @@ class BinaryOperator : public Instruction {
static BinaryOperator *
CreateWithCopiedFlags(BinaryOps Opc, Value *V1, Value *V2, Value *CopyO,
- const Twine &Name = "",
- Instruction *InsertBefore = nullptr) {
+ const Twine &Name,
+ Instruction *InsertBefore) {
BinaryOperator *BO = Create(Opc, V1, V2, Name, InsertBefore);
BO->copyIRFlags(CopyO);
return BO;
}
+ static BinaryOperator *
+ CreateWithCopiedFlags(BinaryOps Opc, Value *V1, Value *V2, Value *CopyO,
+ const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr) {
+ BinaryOperator *BO = Create(Opc, V1, V2, Name, InsertAtEnd);
+ BO->copyIRFlags(CopyO);
+ return BO;
+ }
+
static BinaryOperator *CreateFAddFMF(Value *V1, Value *V2,
Instruction *FMFSource,
const Twine &Name = "") {
@@ -472,22 +497,22 @@ class BinaryOperator : public Instruction {
BasicBlock *InsertAtEnd = nullptr);
static BinaryOperator *CreateNSWNeg(Value *Op, const Twine &Name,
BasicBlock::iterator InsertBefore);
- static BinaryOperator *CreateNSWNeg(Value *Op, const Twine &Name = "",
- Instruction *InsertBefore = nullptr);
static BinaryOperator *CreateNSWNeg(Value *Op, const Twine &Name,
- BasicBlock *InsertAtEnd);
+ Instruction *InsertBefore);
+ static BinaryOperator *CreateNSWNeg(Value *Op, const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr);
static BinaryOperator *CreateNUWNeg(Value *Op, const Twine &Name,
BasicBlock::iterator InsertBefore);
- static BinaryOperator *CreateNUWNeg(Value *Op, const Twine &Name = "",
- Instruction *InsertBefore = nullptr);
static BinaryOperator *CreateNUWNeg(Value *Op, const Twine &Name,
- BasicBlock *InsertAtEnd);
+ Instruction *InsertBefore);
+ static BinaryOperator *CreateNUWNeg(Value *Op, const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr);
static BinaryOperator *CreateNot(Value *Op, const Twine &Name,
BasicBlock::iterator InsertBefore);
- static BinaryOperator *CreateNot(Value *Op, const Twine &Name = "",
- Instruction *InsertBefore = nullptr);
static BinaryOperator *CreateNot(Value *Op, const Twine &Name,
- BasicBlock *InsertAtEnd);
+ Instruction *InsertBefore);
+ static BinaryOperator *CreateNot(Value *Op, const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr);
BinaryOps getOpcode() const {
return static_cast<BinaryOps>(Instruction::getOpcode());
@@ -587,13 +612,13 @@ class CastInst : public UnaryInstruction {
}
/// Constructor with insert-before-instruction semantics for subclasses
CastInst(Type *Ty, unsigned iType, Value *S,
- const Twine &NameStr = "", Instruction *InsertBefore = nullptr)
+ const Twine &NameStr, Instruction *InsertBefore)
: UnaryInstruction(Ty, iType, S, InsertBefore) {
setName(NameStr);
}
/// Constructor with insert-at-end-of-block semantics for subclasses
CastInst(Type *Ty, unsigned iType, Value *S,
- const Twine &NameStr, BasicBlock *InsertAtEnd)
+ const Twine &NameStr = "", BasicBlock *InsertAtEnd = nullptr)
: UnaryInstruction(Ty, iType, S, InsertAtEnd) {
setName(NameStr);
}
@@ -622,8 +647,8 @@ class CastInst : public UnaryInstruction {
Instruction::CastOps, ///< The opcode of the cast instruction
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Provides a way to construct any of the CastInst subclasses using an
/// opcode instead of the subclass's constructor. The opcode must be in the
@@ -635,8 +660,8 @@ class CastInst : public UnaryInstruction {
Instruction::CastOps, ///< The opcode for the cast instruction
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which operand is casted
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create a ZExt or BitCast cast instruction
@@ -651,16 +676,16 @@ class CastInst : public UnaryInstruction {
static CastInst *CreateZExtOrBitCast(
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create a ZExt or BitCast cast instruction
static CastInst *CreateZExtOrBitCast(
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which operand is casted
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create a SExt or BitCast cast instruction
@@ -675,24 +700,24 @@ class CastInst : public UnaryInstruction {
static CastInst *CreateSExtOrBitCast(
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create a SExt or BitCast cast instruction
static CastInst *CreateSExtOrBitCast(
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which operand is casted
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create a BitCast AddrSpaceCast, or a PtrToInt cast instruction.
static CastInst *CreatePointerCast(
Value *S, ///< The pointer value to be casted (operand 0)
Type *Ty, ///< The type to which operand is casted
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create a BitCast, AddrSpaceCast or a PtrToInt cast instruction.
@@ -707,16 +732,16 @@ class CastInst : public UnaryInstruction {
static CastInst *CreatePointerCast(
Value *S, ///< The pointer value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create a BitCast or an AddrSpaceCast cast instruction.
static CastInst *CreatePointerBitCastOrAddrSpaceCast(
Value *S, ///< The pointer value to be casted (operand 0)
Type *Ty, ///< The type to which operand is casted
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create a BitCast or an AddrSpaceCast cast instruction.
@@ -731,8 +756,8 @@ class CastInst : public UnaryInstruction {
static CastInst *CreatePointerBitCastOrAddrSpaceCast(
Value *S, ///< The pointer value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create a BitCast, a PtrToInt, or an IntToPTr cast instruction.
@@ -748,6 +773,19 @@ class CastInst : public UnaryInstruction {
BasicBlock::iterator InsertBefore ///< Place to insert the instruction
);
+ /// Create a BitCast, a PtrToInt, or an IntToPTr cast instruction.
+ ///
+ /// If the value is a pointer type and the destination an integer type,
+ /// creates a PtrToInt cast. If the value is an integer type and the
+ /// destination a pointer type, creates an IntToPtr cast. Otherwise, creates
+ /// a bitcast.
+ static CastInst *CreateBitOrPointerCast(
+ Value *S, ///< The pointer value to be casted (operand 0)
+ Type *Ty, ///< The type to which cast should be made
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
+ );
+
/// Create a BitCast, a PtrToInt, or an IntToPTr cast instruction.
///
/// If the value is a pointer type and the destination an integer type,
@@ -758,7 +796,7 @@ class CastInst : public UnaryInstruction {
Value *S, ///< The pointer value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< Place to insert the instruction
);
/// Create a ZExt, BitCast, or Trunc for int -> int casts.
@@ -775,8 +813,8 @@ class CastInst : public UnaryInstruction {
Value *S, ///< The pointer value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
bool isSigned, ///< Whether to regard S as signed or not
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create a ZExt, BitCast, or Trunc for int -> int casts.
@@ -784,8 +822,8 @@ class CastInst : public UnaryInstruction {
Value *S, ///< The integer value to be casted (operand 0)
Type *Ty, ///< The integer type to which operand is casted
bool isSigned, ///< Whether to regard S as signed or not
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create an FPExt, BitCast, or FPTrunc for fp -> fp casts
@@ -800,16 +838,16 @@ class CastInst : public UnaryInstruction {
static CastInst *CreateFPCast(
Value *S, ///< The floating point value to be casted
Type *Ty, ///< The floating point type to cast to
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create an FPExt, BitCast, or FPTrunc for fp -> fp casts
static CastInst *CreateFPCast(
Value *S, ///< The floating point value to be casted
Type *Ty, ///< The floating point type to cast to
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Create a Trunc or BitCast cast instruction
@@ -824,16 +862,16 @@ class CastInst : public UnaryInstruction {
static CastInst *CreateTruncOrBitCast(
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which cast should be made
- const Twine &Name = "", ///< Name for the instruction
- Instruction *InsertBefore = nullptr ///< Place to insert the instruction
+ const Twine &Name, ///< Name for the instruction
+ Instruction *InsertBefore ///< Place to insert the instruction
);
/// Create a Trunc or BitCast cast instruction
static CastInst *CreateTruncOrBitCast(
Value *S, ///< The value to be casted (operand 0)
Type *Ty, ///< The type to which operand is casted
- const Twine &Name, ///< The name for the instruction
- BasicBlock *InsertAtEnd ///< The block to insert the instruction into
+ const Twine &Name = "", ///< The name for the instruction
+ BasicBlock *InsertAtEnd = nullptr ///< The block to insert the instruction into
);
/// Check whether a bitcast between these types is valid
@@ -1021,13 +1059,13 @@ class CmpInst : public Instruction {
Instruction *FlagsSource = nullptr);
CmpInst(Type *ty, Instruction::OtherOps op, Predicate pred,
- Value *LHS, Value *RHS, const Twine &Name = "",
- Instruction *InsertBefore = nullptr,
+ Value *LHS, Value *RHS, const Twine &Name,
+ Instruction *InsertBefore,
Instruction *FlagsSource = nullptr);
CmpInst(Type *ty, Instruction::OtherOps op, Predicate pred,
- Value *LHS, Value *RHS, const Twine &Name,
- BasicBlock *InsertAtEnd);
+ Value *LHS, Value *RHS, const Twine &Name = "",
+ BasicBlock *InsertAtEnd = nullptr);
public:
// allocate space for exactly two operands
@@ -1048,15 +1086,15 @@ class CmpInst : public Instruction {
/// Create a CmpInst
static CmpInst *Create(OtherOps Op,
Predicate predicate, Value *S1,
- Value *S2, const Twine &Name = "",
- Instruction *InsertBefore = nullptr);
+ Value *S2, const Twine &Name,
+ Instruct...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Mechanically LGTM
The changes look like they move in the direction agreed upon in the Discourse discussion. Making the Instruction* versions not the default makes it easier to deprecate and remove them.
It's a shame that this introduced some almost-duplicated functions in Instruction.cpp - OTOH, we plan to remove one version.
I think it's worth some others having this on their radar too (cc @dwblaikie and @nikic) - I couldn't see this linked in the Discourse thread, it's probably worth dropping a link there if I haven't just missed it.
There was a problem hiding this comment.
I guess this is okay (and I wouldn't want to block this), but this does seem like a good point to reconsider the overall design. I think there are two broad alternatives:
- Only have a single method which takes an InsertionPoint type. This type is basically a union of BasicBlock::iterator, BasicBlock *, or "none". The pro is that we have a good bit less code duplication. The con is that it's a bit less efficient -- though my primary concern would actually the the additional header cost this may impose.
- Remove the "create and insert" support entirely, and instead explicitly call insertBefore() or ... and this is what most code really should be doing ... use IRBuilder. There's a decent chance that code not using IRBuilder also fails to preserve debug locations.
Probably option 2 would be ideal, but it's also the option that involves the most work :)
I agree - the reason I took this approach was mainly that it is the softest in terms of API changes - although we eventually need to move people off of inserting at Instructions, the current approach ensures that as little changes underfoot for API users as possible (although we don't guarantee a stable API, previous discussions indicated that since instruction creation has such a large surface area it should be handled delicately).
I believe this as well - as you mentioned it has extra advantages which are highly relevant to my interests, as I'm imminently looking to make further changes to instruction creation to better preserve debug locations, and these may be easier to implement if we're creating and inserting instructions through the IRBuilder. My major concern however isn't the amount of work required (though it's not to be underestimated), but the disruption that might be involved. Do you have a sense that forcing a transition to IRBuilder would be reasonable? Edit: Also worth noting that while this is a necessary part of step 1, the long term plan is to hopefully swap out all the insert-position arguments with iterators, for which your proposal (1) may be a good intermediate step. |
I don't think we should force the use of IRBuilder everywhere, but I do think that a lot of the code currently doing manual instruction insertion would benefit from using IRBuilder instead. The remainder can still use the explicit insertion methods. But I don't really have a good handle on just how commonly used the create-and-insert overloads currently are (you probably have a better idea, as you did some mass changes in that area...) |
I don't have a great sense of it, since I was mostly detecting and fixing specific patterns rather than getting a sweeping overview. I think a rough outline of a suitable update process though would be:
This means that the only breaking steps are some insert-position arguments being deprecated at step (2), before being removed at step (4), which can be spread out over a release so that all downstream users have a chance to adapt. |
|
@SLTozer That sounds reasonable to me. |
Thanks @nikic - I've created new reviews, #94224 and #94226, which collectively implement the first step of this approach. Rather than use an actual union type, the first patch adds new functionality to the ilist classes to allow us to use just a |
|
Abandoned in favour of #94226. |
This patch changes each of the methods that create instructions and may insert it at the same time - generally
InstrType::Create(...)or constructors - such that the overload called when the insert-position argument is omitted will be the candidate that takes aBasicBlock *InsertAtEndargument, rather than anInstruction *InsertBeforeargument as is currently default in all cases. This is to move us towards deprecating the insert-before-instruction versions, for the sake of preventing debug-info errors creeping in from the RemoveDIs project; for more details on this motivation, see the relevant discourse thread and the pull request that would delete the insert-before-instruction functions.Unclear who's best placed to review this, but since this patch should be NFC - even for downstream consumers - it's relatively low-stakes; for the subsequent patch that deprecates the Instruction versions, I'll put out a RFC on discourse.