[PATCH] Speed up creation of live ranges for physical registers by using a segment set

[Quentin Colombet]

Hi Vaidas,

I share Eric’s concerns/questions.
In particular, I think we should put more effort to reduce the amount of code we duplicate here. I gave some thoughts in the inlined comments.

Also, I think for performance evaluation it would be nice to be able to use or not use it every with a command line option. That would help to figure out what is the problem with computeVirtRegs. Moreover that would allow to completely disable it if we believe it has some problem.

> After a live range is created the contents of the set are flushed to the segment vector, because the set is not as efficient as the vector for the later uses of the live range. After the flushing, the set is deleted and cannot be used again.

If we really want to use the set internally like this, we shouldn’t expose the createSegmentSet method in the public API. Therefore, I think we should try to completely expose this interface. I have thrown some ideas in the inlined comments.

Thanks for working on this!
-Quentin

================
Comment at: include/llvm/CodeGen/LiveInterval.h:194
@@ +193,3 @@
+    typedef std::set<Segment> SegmentSet;
+    SegmentSet* segmentSet;
+
----------------
LLVM policy is to use variable names that start with a capital letter.
That said, the surrounded code already use such a naming convention, so I’d say it is fine.
Anyway, the formatting is not right, because ‘*’ should be with the variable name.

Use clang-format on your patch, please.

================
Comment at: include/llvm/CodeGen/LiveInterval.h:196
@@ +195,3 @@
+
+    LiveRange() : segmentSet(NULL) {
+    }
----------------
Use nullptr.

================
Comment at: include/llvm/CodeGen/LiveInterval.h:200
@@ +199,3 @@
+    ~LiveRange() {
+      if (segmentSet != NULL) {
+        delete segmentSet;
----------------
You do not need to check for NULL before calling delete.

================
Comment at: include/llvm/CodeGen/LiveInterval.h:419
@@ +418,3 @@
+      // segment set is created use it instead of the segments vector
+      if (segmentSet != NULL) {
+        segsetAdd(S);
----------------
Use nullptr (or nothing).

================
Comment at: include/llvm/CodeGen/LiveInterval.h:553
@@ +552,3 @@
+    // on segment set instead of segment vector. These operations
+    // are used during initialization of/ live ranges, if the segment 
+    // set has been activated by createSegmentSet(). 
----------------
Typo: of/ => of

================
Comment at: lib/CodeGen/LiveInterval.cpp:342
@@ +341,3 @@
+  if (segmentSet != NULL) {
+    return segsetExtendInBlock(StartIdx, Kill);
+  }
----------------
I believe this shouldn’t be necessary with some abstractions.
See LiveRange::segsetExtendInBlock.

================
Comment at: lib/CodeGen/LiveInterval.cpp:610
@@ +609,3 @@
+    return I;
+  } else {
+    SegmentSet::iterator PrevI = std::prev(I);
----------------
Please do not use else after return: http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

================
Comment at: lib/CodeGen/LiveInterval.cpp:622
@@ +621,3 @@
+VNInfo *LiveRange::segsetCreateDeadDef(SlotIndex Def,
+                                  VNInfo::Allocator &VNInfoAllocator) {
+  assert(!Def.isDead() && "Cannot define a value at the dead slot");
----------------
Please try to share this code between the vector and the set versions.
You could stick to the existing implementation (with some abstract on the iterator type) and use an abstraction for the final insertion for instance.

================
Comment at: lib/CodeGen/LiveInterval.cpp:656
@@ +655,3 @@
+
+  if (segmentSet->empty()) {
+    return nullptr;
----------------
Same thing as segsetCreateDeadDef, you should share more logic between the implementations.
For instance, you could abstract begin(), upper_bound(), empty() and you should be able to reuse all the code.

================
Comment at: lib/CodeGen/LiveInterval.cpp:677
@@ +676,3 @@
+// a variant of extendSegmentEndTo working with the segment set instead of the segment vector
+void LiveRange::segsetExtendSegmentEndTo(LiveRange::SegmentSet::iterator I, SlotIndex NewEnd) {
+  assert(I != segmentSet->end() && "Not a valid segment!");
----------------
Ditto.
Abstract end(), erase().

================
Comment at: lib/CodeGen/LiveInterval.cpp:948
@@ +947,3 @@
+  if (LR->segmentSet != NULL) {
+    LR->addSegment(Seg);
+    return;
----------------
Is this code path used during createDeadDefs and extendToUses?

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:297
@@ -296,1 +296,3 @@
+          // use segment set to speed-up initial computation of the live range
+          LR->createSegmentSet();
           NewRanges.push_back(Unit);
----------------
I would prefer this to be an internal detail of the constructor of LiveRange.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:315
@@ -311,1 +314,3 @@
+    // flush the segment set to array and destroy it
+    LR->flushSegmentSet();
   }
----------------
What about pushing the flush into computeRegUnitRange?

Even better, could we automatically decide when to do the flush internally in the LiveRange class? E.g., the first time we actually need the vector capabilities.
That would remove this kind of explicit calls to flush that are easy to forget or misplace.

http://reviews.llvm.org/D6013