From 0d3dc6c77748433a2c85dad3c40fd5d619c7192e Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 13:19:16 +0100 Subject: [PATCH 1/6] [ArrayRef] Add constructor from iterator_range (NFC). Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. I will share a follow-up soon. --- llvm/include/llvm/ADT/ArrayRef.h | 13 +++++++++++++ llvm/unittests/ADT/ArrayRefTest.cpp | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index a1317423cdd1a..f2fc7b636a8f5 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -149,6 +149,19 @@ namespace llvm { * = nullptr) : Data(Vec.data()), Length(Vec.size()) {} + /// Construct an ArrayRef from iterator_range. This uses SFINAE + /// to ensure that this is only used for iterator ranges of random access + /// iterators that can be converted. + template + ArrayRef(const iterator_range &Range, + std::enable_if_t:: + iterator_category>::value && + std::is_convertible::value, + void> * = nullptr) + : Data(Range.begin()), Length(llvm::size(Range)) {} + /// @} /// @name Simple Operations /// @{ diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index fb25ee19c0b20..128efbea813d2 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -255,6 +255,26 @@ TEST(ArrayRefTest, ArrayRefFromStdArray) { } } +TEST(ArrayRefTest, ArrayRefFromIteratorRange) { + std::array A1{{42, -5, 0, 1000000, -1000000}}; + ArrayRef A2 = make_range(A1.begin(), A1.end()); + + EXPECT_EQ(A1.size(), A2.size()); + for (std::size_t i = 0; i < A1.size(); ++i) { + EXPECT_EQ(A1[i], A2[i]); + } +} + +TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) { + std::array A1{{42, -5, 0, 1000000, -1000000}}; + ArrayRef A2 = make_range(A1.begin(), A1.end()); + + EXPECT_EQ(A1.size(), A2.size()); + for (std::size_t i = 0; i < A1.size(); ++i) { + EXPECT_EQ(A1[i], A2[i]); + } +} + static_assert(std::is_trivially_copyable_v>, "trivially copyable"); From ce3bc7cfb83093d879eebdc6280842b9042e58dc Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 17:40:23 +0100 Subject: [PATCH 2/6] !fixup address latest comments, thanks! --- llvm/include/llvm/ADT/ArrayRef.h | 17 +++++++++-------- llvm/unittests/ADT/ArrayRefTest.cpp | 6 ++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index f2fc7b636a8f5..47ba9fe3a23e3 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -152,14 +152,15 @@ namespace llvm { /// Construct an ArrayRef from iterator_range. This uses SFINAE /// to ensure that this is only used for iterator ranges of random access /// iterators that can be converted. - template - ArrayRef(const iterator_range &Range, - std::enable_if_t:: - iterator_category>::value && - std::is_convertible::value, - void> * = nullptr) + template &>() + .begin())>::iterator_category>::value && + std::is_convertible_v>> + ArrayRef(const iterator_range &Range) : Data(Range.begin()), Length(llvm::size(Range)) {} /// @} diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index 128efbea813d2..098a0910b6cb8 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -260,9 +260,8 @@ TEST(ArrayRefTest, ArrayRefFromIteratorRange) { ArrayRef A2 = make_range(A1.begin(), A1.end()); EXPECT_EQ(A1.size(), A2.size()); - for (std::size_t i = 0; i < A1.size(); ++i) { + for (std::size_t i = 0; i < A1.size(); ++i) EXPECT_EQ(A1[i], A2[i]); - } } TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) { @@ -270,9 +269,8 @@ TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) { ArrayRef A2 = make_range(A1.begin(), A1.end()); EXPECT_EQ(A1.size(), A2.size()); - for (std::size_t i = 0; i < A1.size(); ++i) { + for (std::size_t i = 0; i < A1.size(); ++i) EXPECT_EQ(A1[i], A2[i]); - } } static_assert(std::is_trivially_copyable_v>, From 1a8430fb5df52f46eaab9ac3e0592c9d2af7c9d8 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 17:43:40 +0100 Subject: [PATCH 3/6] !fixup enable_if --- llvm/include/llvm/ADT/ArrayRef.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index 47ba9fe3a23e3..c380f71832d13 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -153,7 +153,7 @@ namespace llvm { /// to ensure that this is only used for iterator ranges of random access /// iterators that can be converted. template Date: Tue, 29 Apr 2025 19:18:51 +0100 Subject: [PATCH 4/6] !fixup simplify SFINAE check --- llvm/include/llvm/ADT/ArrayRef.h | 13 +++---------- llvm/unittests/ADT/ArrayRefTest.cpp | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index c380f71832d13..4f5567ecc55b7 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -150,16 +150,9 @@ namespace llvm { : Data(Vec.data()), Length(Vec.size()) {} /// Construct an ArrayRef from iterator_range. This uses SFINAE - /// to ensure that this is only used for iterator ranges of random access - /// iterators that can be converted. - template &>() - .begin())>::iterator_category>::value && - std::is_convertible_v>> + /// to ensure that this is only used for iterator ranges over plain pointer + /// iterators. + template >> ArrayRef(const iterator_range &Range) : Data(Range.begin()), Length(llvm::size(Range)) {} diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index 098a0910b6cb8..f6f36e5a881e1 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -255,6 +255,20 @@ TEST(ArrayRefTest, ArrayRefFromStdArray) { } } +struct TestRandomAccessIterator { + using iterator_category = std::random_access_iterator_tag; +}; + +static_assert( + !std::is_constructible, + iterator_range>::value, + "cannot construct from iterator range with non-pointer iterator"); +static_assert(!std::is_constructible, iterator_range>::value, + "cannot construct from iterator range with non-pointer iterator"); +static_assert( + std::is_constructible, iterator_range>::value, + "should be able to construct ArrayRef from iterator_range over pointers"); + TEST(ArrayRefTest, ArrayRefFromIteratorRange) { std::array A1{{42, -5, 0, 1000000, -1000000}}; ArrayRef A2 = make_range(A1.begin(), A1.end()); From e928962eb6f3410208fe222e2ea10d4fdf64c6a5 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 13:24:33 +0100 Subject: [PATCH 5/6] [VPlan] Consistently use ArrayRef for operands in ctors (NFC) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on https://github.com/llvm/llvm-project/pull/137796. --- .../Transforms/Vectorize/LoopVectorize.cpp | 21 +++----- llvm/lib/Transforms/Vectorize/VPlan.h | 53 ++++++------------- 2 files changed, 25 insertions(+), 49 deletions(-) diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp index 4684378687ef6..8541968b98834 100644 --- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp +++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp @@ -8749,7 +8749,7 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(Instruction *I, Plan.getOrAddLiveIn(ConstantInt::get(I->getType(), 1u, false)); auto *SafeRHS = Builder.createSelect(Mask, Ops[1], One, I->getDebugLoc()); Ops[1] = SafeRHS; - return new VPWidenRecipe(*I, make_range(Ops.begin(), Ops.end())); + return new VPWidenRecipe(*I, Ops); } [[fallthrough]]; } @@ -8795,7 +8795,7 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(Instruction *I, // For other binops, the legacy cost model only checks the second operand. NewOps[1] = GetConstantViaSCEV(NewOps[1]); } - return new VPWidenRecipe(*I, make_range(NewOps.begin(), NewOps.end())); + return new VPWidenRecipe(*I, NewOps); } case Instruction::ExtractValue: { SmallVector NewOps(Operands); @@ -8804,7 +8804,7 @@ VPWidenRecipe *VPRecipeBuilder::tryToWiden(Instruction *I, assert(EVI->getNumIndices() == 1 && "Expected one extractvalue index"); unsigned Idx = EVI->getIndices()[0]; NewOps.push_back(Plan.getOrAddLiveIn(ConstantInt::get(I32Ty, Idx, false))); - return new VPWidenRecipe(*I, make_range(NewOps.begin(), NewOps.end())); + return new VPWidenRecipe(*I, NewOps); } }; } @@ -8828,9 +8828,7 @@ VPRecipeBuilder::tryToWidenHistogram(const HistogramInfo *HI, if (Legal->isMaskRequired(HI->Store)) HGramOps.push_back(getBlockInMask(HI->Store->getParent())); - return new VPHistogramRecipe(Opcode, - make_range(HGramOps.begin(), HGramOps.end()), - HI->Store->getDebugLoc()); + return new VPHistogramRecipe(Opcode, HGramOps, HI->Store->getDebugLoc()); } VPReplicateRecipe * @@ -8891,8 +8889,7 @@ VPRecipeBuilder::handleReplication(Instruction *I, ArrayRef Operands, assert((Range.Start.isScalar() || !IsUniform || !IsPredicated || (Range.Start.isScalable() && isa(I))) && "Should not predicate a uniform recipe"); - auto *Recipe = new VPReplicateRecipe( - I, make_range(Operands.begin(), Operands.end()), IsUniform, BlockInMask); + auto *Recipe = new VPReplicateRecipe(I, Operands, IsUniform, BlockInMask); return Recipe; } @@ -9081,12 +9078,10 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe( return nullptr; if (auto *GEP = dyn_cast(Instr)) - return new VPWidenGEPRecipe(GEP, - make_range(Operands.begin(), Operands.end())); + return new VPWidenGEPRecipe(GEP, Operands); if (auto *SI = dyn_cast(Instr)) { - return new VPWidenSelectRecipe( - *SI, make_range(Operands.begin(), Operands.end())); + return new VPWidenSelectRecipe(*SI, Operands); } if (auto *CI = dyn_cast(Instr)) { @@ -9117,7 +9112,7 @@ VPRecipeBuilder::tryToCreatePartialReduction(Instruction *Reduction, SmallVector Ops; Ops.push_back(Plan.getOrAddLiveIn(Zero)); Ops.push_back(BinOp); - BinOp = new VPWidenRecipe(*Reduction, make_range(Ops.begin(), Ops.end())); + BinOp = new VPWidenRecipe(*Reduction, Ops); Builder.insert(BinOp->getDefiningRecipe()); ReductionOpcode = Instruction::Add; } diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h index 067a723a3aa4d..446fb117ec2f8 100644 --- a/llvm/lib/Transforms/Vectorize/VPlan.h +++ b/llvm/lib/Transforms/Vectorize/VPlan.h @@ -386,10 +386,6 @@ class VPRecipeBase : public ilist_node_with_parent, DebugLoc DL = {}) : VPDef(SC), VPUser(Operands), DL(DL) {} - template - VPRecipeBase(const unsigned char SC, iterator_range Operands, - DebugLoc DL = {}) - : VPDef(SC), VPUser(Operands), DL(DL) {} virtual ~VPRecipeBase() = default; /// Clone the current recipe. @@ -504,17 +500,12 @@ class VPRecipeBase : public ilist_node_with_parent, /// Note that VPRecipeBase must be inherited from before VPValue. class VPSingleDefRecipe : public VPRecipeBase, public VPValue { public: - template - VPSingleDefRecipe(const unsigned char SC, IterT Operands, DebugLoc DL = {}) - : VPRecipeBase(SC, Operands, DL), VPValue(this) {} - VPSingleDefRecipe(const unsigned char SC, ArrayRef Operands, DebugLoc DL = {}) : VPRecipeBase(SC, Operands, DL), VPValue(this) {} - template - VPSingleDefRecipe(const unsigned char SC, IterT Operands, Value *UV, - DebugLoc DL = {}) + VPSingleDefRecipe(const unsigned char SC, ArrayRef Operands, + Value *UV, DebugLoc DL = {}) : VPRecipeBase(SC, Operands, DL), VPValue(this, UV) {} static inline bool classof(const VPRecipeBase *R) { @@ -648,15 +639,15 @@ class VPRecipeWithIRFlags : public VPSingleDefRecipe { } public: - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, DebugLoc DL = {}) + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, + DebugLoc DL = {}) : VPSingleDefRecipe(SC, Operands, DL) { OpType = OperationType::Other; AllFlags = 0; } - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, Instruction &I) + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, + Instruction &I) : VPSingleDefRecipe(SC, Operands, &I, I.getDebugLoc()) { if (auto *Op = dyn_cast(&I)) { OpType = OperationType::Cmp; @@ -685,33 +676,28 @@ class VPRecipeWithIRFlags : public VPSingleDefRecipe { } } - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, CmpInst::Predicate Pred, DebugLoc DL = {}) : VPSingleDefRecipe(SC, Operands, DL), OpType(OperationType::Cmp), CmpPredicate(Pred) {} - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, WrapFlagsTy WrapFlags, DebugLoc DL = {}) : VPSingleDefRecipe(SC, Operands, DL), OpType(OperationType::OverflowingBinOp), WrapFlags(WrapFlags) {} - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, FastMathFlags FMFs, DebugLoc DL = {}) : VPSingleDefRecipe(SC, Operands, DL), OpType(OperationType::FPMathOp), FMFs(FMFs) {} - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, DisjointFlagsTy DisjointFlags, DebugLoc DL = {}) : VPSingleDefRecipe(SC, Operands, DL), OpType(OperationType::DisjointOp), DisjointFlags(DisjointFlags) {} protected: - template - VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, + VPRecipeWithIRFlags(const unsigned char SC, ArrayRef Operands, GEPNoWrapFlags GEPFlags, DebugLoc DL = {}) : VPSingleDefRecipe(SC, Operands, DL), OpType(OperationType::GEPOp), GEPFlags(GEPFlags) {} @@ -1225,15 +1211,13 @@ class VPWidenRecipe : public VPRecipeWithIRFlags, public VPIRMetadata { unsigned Opcode; protected: - template VPWidenRecipe(unsigned VPDefOpcode, Instruction &I, - iterator_range Operands) + ArrayRef Operands) : VPRecipeWithIRFlags(VPDefOpcode, Operands, I), VPIRMetadata(I), Opcode(I.getOpcode()) {} public: - template - VPWidenRecipe(Instruction &I, iterator_range Operands) + VPWidenRecipe(Instruction &I, ArrayRef Operands) : VPWidenRecipe(VPDef::VPWidenSC, I, Operands) {} ~VPWidenRecipe() override = default; @@ -1466,8 +1450,7 @@ class VPHistogramRecipe : public VPRecipeBase { unsigned Opcode; public: - template - VPHistogramRecipe(unsigned Opcode, iterator_range Operands, + VPHistogramRecipe(unsigned Opcode, ArrayRef Operands, DebugLoc DL = {}) : VPRecipeBase(VPDef::VPHistogramSC, Operands, DL), Opcode(Opcode) {} @@ -1503,8 +1486,7 @@ class VPHistogramRecipe : public VPRecipeBase { /// A recipe for widening select instructions. struct VPWidenSelectRecipe : public VPRecipeWithIRFlags, public VPIRMetadata { - template - VPWidenSelectRecipe(SelectInst &I, iterator_range Operands) + VPWidenSelectRecipe(SelectInst &I, ArrayRef Operands) : VPRecipeWithIRFlags(VPDef::VPWidenSelectSC, Operands, I), VPIRMetadata(I) {} @@ -1563,8 +1545,7 @@ class VPWidenGEPRecipe : public VPRecipeWithIRFlags { } public: - template - VPWidenGEPRecipe(GetElementPtrInst *GEP, iterator_range Operands) + VPWidenGEPRecipe(GetElementPtrInst *GEP, ArrayRef Operands) : VPRecipeWithIRFlags(VPDef::VPWidenGEPSC, Operands, *GEP) { SmallVector> Metadata; (void)Metadata; @@ -2487,7 +2468,7 @@ class VPReplicateRecipe : public VPRecipeWithIRFlags { public: template - VPReplicateRecipe(Instruction *I, iterator_range Operands, + VPReplicateRecipe(Instruction *I, ArrayRef Operands, bool IsUniform, VPValue *Mask = nullptr) : VPRecipeWithIRFlags(VPDef::VPReplicateSC, Operands, *I), IsUniform(IsUniform), IsPredicated(Mask) { From 925932689903ba141e67e68ce4f81b416bfe074f Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 22:11:57 +0100 Subject: [PATCH 6/6] !fixup remove left over template arg. --- llvm/lib/Transforms/Vectorize/VPlan.h | 1 - 1 file changed, 1 deletion(-) diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h index 446fb117ec2f8..de840763b5ca9 100644 --- a/llvm/lib/Transforms/Vectorize/VPlan.h +++ b/llvm/lib/Transforms/Vectorize/VPlan.h @@ -2467,7 +2467,6 @@ class VPReplicateRecipe : public VPRecipeWithIRFlags { bool IsPredicated; public: - template VPReplicateRecipe(Instruction *I, ArrayRef Operands, bool IsUniform, VPValue *Mask = nullptr) : VPRecipeWithIRFlags(VPDef::VPReplicateSC, Operands, *I),