[PATCH] D20507: CodeGen: Refactor renameDisconnectedComponents() as a pass

[Quentin Colombet via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

Hi Matthias,

LGTM.

Nitpicks inlined.

Cheers,
-Quentin


================
Comment at: lib/CodeGen/LiveRangeUtils.h:10
@@ +9,3 @@
+//
+/// This file contains helper function to modify live ranges.
+//
----------------
typo: functions

================
Comment at: lib/CodeGen/LiveRangeUtils.h:22
@@ +21,3 @@
+/// Helper function to to split live segments assigned to different classes to
+/// new live range objects.
+template<typename LiveRangeT, typename EqClassesT>
----------------
typo: to to

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:1
@@ +1,2 @@
+//===-- LiveIntervalAnalysis.cpp - Live Interval Analysis -----------------===//
+//
----------------
Copy/paste :)

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:40
@@ +39,3 @@
+
+using namespace llvm;
+
----------------
Could you add a DEBUG_TYPE and a few DEBUG statements in the pass to follow what is going on?

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:107
@@ +106,3 @@
+
+} // end anonymous namespace
+
----------------
Period

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:122
@@ +121,3 @@
+  // Shortcut: We cannot have split components with a single definition.
+  if (LI.valnos.size() < 2)
+    return false;
----------------
DEBUG comment.
Like we could print the register here.

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:127
@@ +126,3 @@
+  IntEqClasses Classes;
+  if (!findComponents(Classes, SubRangeInfos, LI))
+    return false;
----------------
DEBUG comment.
And add a comment here to say we are going to check if that live-range has several component, etc.

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:137
@@ +136,3 @@
+       ++I) {
+    unsigned NewVReg = MRI->createVirtualRegister(RegClass);
+    LiveInterval &NewLI = LIS->createEmptyInterval(NewVReg);
----------------
Although the current implementation is correct, I was wondering what are your thoughts regarding relaxing the constraints on the register class and constraining them back when we rewrite the operands?
I actually do not expect that we would get much more freedom for regalloc afterwards, but who knows.

Note: the current implementation is the right thing to do, I was thinking in terms of follow-up patch.

================
Comment at: lib/CodeGen/RenameIndependentSubregs.cpp:201
@@ +200,3 @@
+  unsigned NumClasses = Classes.getNumClasses();
+  return NumClasses > 1;
+}
----------------
DEBUG comment.
Print that number.

================
Comment at: test/CodeGen/AMDGPU/rename-independent-subregs.mir:6
@@ +5,3 @@
+---
+# CHECK-LABEL: name: test0
+# CHECK: S_NOP 0, implicit-def undef %0:sub0
----------------
Could you add a comment explaining what configuration does this test check?
Basically, I would expecting something stating that this test checks that we correctly split %0 as it has three disjoint sets.


Repository:
  rL LLVM

http://reviews.llvm.org/D20507