[PATCH] ADT SortedVector and MachineBasicBlock::LiveIns changes.

[Quentin Colombet]

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
>> 
>