[PATCH] ADT SortedVector and MachineBasicBlock::LiveIns changes.
Puyan Lotfi
plotfi at apple.com
Sun Jul 26 16:03:04 PDT 2015
Hi Quentin
I agree on all fronts.
I have updated patches that pretty much leave SortedVector the same but
allow the client MachineBasicBlock to handle the sorting on the
iterators. I agree that this is probably the most reasonable tradeoff.
PL
On Thu, Jul 23, 2015 at 02:16:02PM -0700, Quentin Colombet wrote:
> Hi Puyan,
>
> Thanks for the updated patch!
>
> On a second thought, the long term solution shouldn’t involve the clients of MachineBasicBlock to have to think about calling sortUniqueLiveIns in the right places.
> The basic block should handle that internally.
> E.g., things like asking for the iterators should sort and unique the vector, but things like empty, addLiveIn, isLiveIn shouldn’t.
>
>
> Couple of comments:
> * SortedVector_Target.patch
>
> +++ b/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
> + for (auto BI = MF->begin(), BE = MF->end(); BI != BE; ++BI)
> + BI->sortUniqueLiveIns();
> +
>
> This is loop should be pushed in the previous if block, shouldn’t it?
> Also, like I said in the beginning of this email, we shouldn’t have to add this call.
> My concern is that between the call to addLiveIn and the call that fixes the set, aside from careful review from the developer, we do not have any idea whether or not the set will be used by some internal APIs.
>
> +++ b/lib/Target/Mips/MipsSEFrameLowering.cpp
> - if (!MBB.isLiveIn(ABI.GetEhDataReg(I)))
> - MBB.addLiveIn(ABI.GetEhDataReg(I));
> + MBB.addLiveIn(ABI.GetEhDataReg(I));
>
> This is not consistent with how you’ve patched the previous isLiveIn/addLiveIn call sites so far.
>
> I would recommend making that consistent and I believe removing the check for all of them is the right call (does not mean addLiveIn cannot be smart and check is the thing is not in when the set is already sorted to avoid to invalid the state).
>
>
> +++ b/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
> @@ -143,6 +143,7 @@ void MipsSEDAGToDAGISel::initGlobalBaseReg(MachineFunction &MF) {
> if (ABI.IsN64()) {
> MF.getRegInfo().addLiveIn(Mips::T9_64);
> MBB.addLiveIn(Mips::T9_64);
> + MBB.sortUniqueLiveIns();
>
> Shouldn’t be needed if handled transparently by the basic block.
>
> Ditto for all the subsequent call to sortUniqueLiveIns.
>
> * SortedVector_CodeGen.patch
>
> Shouldn’t be needed at all.
>
> * SortedVector_ADT
>
> + iterator begin() {
> + doCheck();
> + return Vector.begin();
>
> Instead of doCheck, we should sortUnique here.
> Same for operator[], size, and others iterators.
>
>
> > On Jul 22, 2015, at 4:40 PM, Puyan Lotfi <plotfi at apple.com> wrote:
> >
> >
> > Thanks Quentin!
> >
> >> On Jul 15, 2015, at 3:31 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> >>
> >> Hi Puyan,
> >>
> >> Thanks for working on this!
> >>
> >> A couple of high level questions/remarks:
> >> What are the compile time figures with those patches?
> >>
> >
> > I’ve not measured yet; do you think I should necesarily do this prior to submitting?
> > Based on the size of the LiveIns vectors I’ve observed, I doubt this is something I need to do immediately.
>
> Agreed.
>
> Thanks again,
> -Quentin
> >
> >> I think the SortedVector should have a Compare functor to be part of the ADT.
> >>
> >
> > Done.
> >
> >>
> >> + // Sort and Unique the LiveIns just in case.
> >> + MBB->sortUniqueLiveIns();
> >> +
> >>
> >> Shouldn’t this be a an invariant of the BasicBlock?
> >> Have you seen that being false at some point? (Other than when we are building the sets?)
> >>
> >
> > Sorry, this was accidentally leftover. Removed, and added the proper sortUniqueLiveIns call in one more place where we do an insert.
> >
> >>
> >> Nitpick:
> >> + bool verify() const {
> >> + for (unsigned i = 1; i < Vector.size(); i++)
> >> + if (Vector[i - 1] >= Vector[i])
> >> + return false;
> >> + return true;
> >> + }
> >>
> >> Shouldn’t this check occur only when IsSorted is true or at least check that IsSorted is true?
> >>
> >
> > Yeah, this is simpler. I wanted something that was more of a brute force check for verify but I prefer the simplicity.
> >
> >> Thanks,
> >> -Quentin
> >>
> >
> > Newer patches based on these suggestions:
> >
> >
> > <SortedVector_Target.patch><SortedVector_CodeGen.patch><SortedVector_ADT.patch>
> >
> > PL
> >
> >
> >>
> >>> On Jul 15, 2015, at 1:58 PM, Puyan Lotfi <plotfi at apple.com> wrote:
> >>>
> >>> All:
> >>>
> >>> The following patches include a new ADT I’d like to add to LLVM: the SortedVector.
> >>> SortedVector is a std::vector that enforces sorted ordering (ascending for now) and uniqueness on access but allows for insertions to be unsorted.
> >>>
> >>> The reason I implemented this is because I noticed that MachineBasicBlock LiveIns are not actually sorted or unique even though they are normally almost sorted and almost unique.
> >>> So by implementing this ADT and by also adding a machine verifier check they will actually be sorted and unique on access.
> >>>
> >>> I’ve split my patch into:
> >>>
> >>> - The SortedVector ADT
> >>> - Changes to MachineBasicBlock
> >>> - Changes to files in lib/CodeGen
> >>> - Changes to the targets
> >>> - An added machine verifier check
> >>>
> >>> Most of the changes to lib/CodeGen and the Targets are just calls to the sorting method usually after a loop where many LiveIns are added to the MachineBasicBlock::LiveIns vector.
> >>>
> >>> Thanks
> >>>
> >>> PL
> >>>
> >>>
> >>> <SortedVector_ADT.patch><SortedVector_CodeGen.patch><SortedVector_MachineBasicBlock.patch><SortedVector_MachineVerifier.patch><SortedVector_Target.patch>_______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >
>
-------------- next part --------------
diff --git a/include/llvm/ADT/SortedVector.h b/include/llvm/ADT/SortedVector.h
new file mode 100644
index 0000000..1fd4cfc
--- /dev/null
+++ b/include/llvm/ADT/SortedVector.h
@@ -0,0 +1,133 @@
+//===-- llvm/ADT/SortedVector.h ---------------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ADT_SORTEDVECTOR_H
+#define LLVM_ADT_SORTEDVECTOR_H
+
+#include <vector>
+#include <cassert>
+#include <functional>
+#include "llvm/Support/raw_ostream.h"
+
+namespace llvm {
+
+/// \brief Lazily maintains a sorted and unique vector of elements of type T.
+template<typename T, typename CMP = std::less<T>>
+class SortedVector {
+public:
+ typedef typename std::vector<T> VectorType;
+ typedef typename VectorType::iterator iterator;
+ typedef typename VectorType::const_iterator const_iterator;
+
+private:
+ VectorType Vector;
+ bool IsSorted = true;
+
+ void doCheck() const {
+ assert(IsSorted && "Unsorted SortedVector access; call sortUnique prior.");
+ }
+
+public:
+ /// \brief Appends Entry to the sorted unique vector; sets the IsSorted flag
+ /// to false if appending Entry puts Vector into an unsorted state.
+ void insert(const T &Entry) {
+ if (!Vector.size())
+ Vector.push_back(Entry);
+
+ // Vector is sorted and Entry is a duplicate of the previous so skip.
+ if (IsSorted && Entry == Vector.back())
+ return;
+
+ IsSorted &= (CMP()(Vector.back(), Entry));
+ Vector.push_back(Entry);
+ }
+
+ // \brief Sorts and uniques Vector.
+ void sortUnique() {
+ if (IsSorted)
+ return;
+
+ std::sort(Vector.begin(), Vector.end());
+ Vector.erase(std::unique(Vector.begin(), Vector.end()), Vector.end());
+ IsSorted = true;
+ }
+
+ /// \brief Tells if Entry is in Vector without relying on sorted-uniqueness.
+ bool has(T Entry) const {
+ if (IsSorted)
+ return std::binary_search(Vector.begin(), Vector.end(), Entry);
+
+ return std::find(Vector.begin(), Vector.end(), Entry) != Vector.end();
+ }
+
+ /// \brief Returns a reference to the entry with the specified index.
+ const T &operator[](unsigned index) const {
+ assert(index < size() && "SortedVector index is out of range!");
+ doCheck();
+ return Vector[index];
+ }
+
+ /// \brief Return an iterator to the start of the vector.
+ iterator begin() {
+ doCheck();
+ return Vector.begin();
+ }
+
+ /// \brief Returns const iterator to the start of the vector.
+ const_iterator begin() const {
+ doCheck();
+ return Vector.begin();
+ }
+
+ /// \brief Returns iterator to the end of Vector.
+ iterator end() {
+ doCheck();
+ return Vector.end();
+ }
+
+ /// \brief Returns const iterator to the end of Vector. Assert if unsorted.
+ const_iterator end() const {
+ doCheck();
+ return Vector.end();
+ }
+
+ /// \brief Erases Vector at position. Asserts if Vector is unsorted.
+ iterator erase(iterator position) {
+ doCheck();
+ return Vector.erase(position);
+ }
+
+ /// \brief Erases Vector entirely.
+ iterator erase() {
+ IsSorted = true;
+ return Vector.erase();
+ }
+
+ /// \brief Returns number of entries in Vector; asserts if it is unsorted.
+ size_t size() const {
+ doCheck();
+ return Vector.size();
+ }
+
+ /// \brief Returns true if Vector is empty.
+ bool empty() const {
+ return Vector.empty();
+ }
+
+ /// \brief Clears all the entries.
+ void reset() {
+ IsSorted = true;
+ Vector.resize(0, 0);
+ }
+};
+
+} // End of namespace llvm
+
+#endif // LLVM_ADT_SORTEDVECTOR_H
+
-------------- next part --------------
diff --git a/include/llvm/CodeGen/MachineBasicBlock.h b/include/llvm/CodeGen/MachineBasicBlock.h
index 5e5f45c..ecb5a74 100644
--- a/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/include/llvm/CodeGen/MachineBasicBlock.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CODEGEN_MACHINEBASICBLOCK_H
#define LLVM_CODEGEN_MACHINEBASICBLOCK_H
+#include "llvm/ADT/SortedVector.h"
#include "llvm/ADT/GraphTraits.h"
#include "llvm/CodeGen/MachineInstr.h"
#include "llvm/Support/DataTypes.h"
@@ -80,7 +81,7 @@ class MachineBasicBlock : public ilist_node<MachineBasicBlock> {
/// LiveIns - Keep track of the physical registers that are livein of
/// the basicblock.
- std::vector<unsigned> LiveIns;
+ mutable SortedVector<unsigned> LiveIns;
/// Alignment - Alignment of the basic block. Zero if the basic block does
/// not need to be aligned.
@@ -318,15 +319,12 @@ public:
/// Adds the specified register as a live in. Note that it is an error to add
/// the same register to the same set more than once unless the intention is
/// to call sortUniqueLiveIns after all registers are added.
- void addLiveIn(unsigned Reg) { LiveIns.push_back(Reg); }
+ void addLiveIn(unsigned Reg) { LiveIns.insert(Reg); }
/// Sorts and uniques the LiveIns vector. It can be significantly faster to do
/// this than repeatedly calling isLiveIn before calling addLiveIn for every
/// LiveIn insertion.
- void sortUniqueLiveIns() {
- std::sort(LiveIns.begin(), LiveIns.end());
- LiveIns.erase(std::unique(LiveIns.begin(), LiveIns.end()), LiveIns.end());
- }
+ void sortUniqueLiveIns() { LiveIns.sortUnique(); }
/// Add PhysReg as live in to this block, and ensure that there is a copy of
/// PhysReg to a virtual register of class RC. Return the virtual register
@@ -339,14 +337,20 @@ public:
/// isLiveIn - Return true if the specified register is in the live in set.
///
- bool isLiveIn(unsigned Reg) const;
+ bool isLiveIn(unsigned Reg) const { return LiveIns.has(Reg); }
// Iteration support for live in sets. These sets are kept in sorted
// order by their register number.
typedef std::vector<unsigned>::const_iterator livein_iterator;
- livein_iterator livein_begin() const { return LiveIns.begin(); }
- livein_iterator livein_end() const { return LiveIns.end(); }
bool livein_empty() const { return LiveIns.empty(); }
+ livein_iterator livein_begin() const {
+ LiveIns.sortUnique();
+ return LiveIns.begin();
+ }
+ livein_iterator livein_end() const {
+ LiveIns.sortUnique();
+ return LiveIns.end();
+ }
/// getAlignment - Return alignment of the basic block.
/// The alignment is specified as log2(bytes).
diff --git a/lib/CodeGen/MachineBasicBlock.cpp b/lib/CodeGen/MachineBasicBlock.cpp
index 5d3f7eb..f60e6bd 100644
--- a/lib/CodeGen/MachineBasicBlock.cpp
+++ b/lib/CodeGen/MachineBasicBlock.cpp
@@ -332,11 +332,6 @@ void MachineBasicBlock::removeLiveIn(unsigned Reg) {
LiveIns.erase(I);
}
-bool MachineBasicBlock::isLiveIn(unsigned Reg) const {
- livein_iterator I = std::find(livein_begin(), livein_end(), Reg);
- return I != livein_end();
-}
-
unsigned
MachineBasicBlock::addLiveIn(unsigned PhysReg, const TargetRegisterClass *RC) {
assert(getParent() && "MBB must be inserted in function");
More information about the llvm-commits
mailing list