[PATCH] Early expansion of MUX instructions on Hexagon

[Quentin Colombet]

Hi Krzysztof,

Thanks for your patience.

The code that updates the liveness seems good to me. You have duplicated some functionality from MachineInstr that I pointed out. Moreover, I believe more comments on the functions themselves would be general goodness.

I admit I have been less careful for the end of the review (starting at HexagonExpandCondsets::removeInstrFromLiveness), since you showed you understand the underlying implications on the liveness.
That being said, make sure that the test cases you are adding cover all the liveness corner cases you handle. Right now, I am a bit dubious that you do not need more of them. You know more than me how all that is being invoked, though :).

Note: I haven’t check at all the logic for the optimization itself.

Cheers,
-Quentin


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:258
@@ +257,3 @@
+
+void HexagonExpandCondsets::makeDefined(unsigned Reg, SlotIndex S,
+      bool SetDef) {
----------------
I think this function deserves a comment.
>From the name, it is not clear that this update the undef flag of the uses of Reg at S.

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:264
@@ +263,3 @@
+  assert(MI && "Expecting instruction");
+  for (MIOperands Mo(MI); Mo.isValid(); ++Mo) {
+    if (!Mo->isReg() || !Mo->isUse() || Mo->getReg() != Reg)
----------------
You can use for-range based loop for that.

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:299
@@ +298,3 @@
+        continue;
+      if (Mo->isDead())
+        Mo->setIsDead(false);
----------------
This if is probably worth for performances than always setting the flag to false.

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:328
@@ +327,3 @@
+        continue;
+      if (Mo->isDead())
+        Mo->setIsDead(false);
----------------
Ditto.

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:346
@@ +345,3 @@
+
+void HexagonExpandCondsets::clearKillFlags(MachineInstr *MI) {
+  for (MIOperands Mo(MI); Mo.isValid(); ++Mo)
----------------
This functionality is already available via MachineInstr::clearKillInfo().

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:353
@@ +352,3 @@
+
+void HexagonExpandCondsets::resetKillFlags(unsigned Reg, LiveInterval &LI) {
+  MRI->clearKillFlags(Reg);
----------------
This method would deserve a comment as well and/or a renaming.

=> You mark all the uses as kill.

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:370
@@ +369,3 @@
+
+void HexagonExpandCondsets::terminateSegment(LiveInterval::iterator LT,
+      SlotIndex S, LiveInterval &LI) {
----------------
Would deserve a comment.

In particular explain that you create holes in the live-range and how do you plan to fix them.

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:451
@@ +450,3 @@
+      EX = N->start.isBlock() ? N->start.getPrevIndex() : N->start;
+    }
+    if (Predicated) {
----------------
Okay, I see where you fill the holes now :).

================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:527
@@ +526,3 @@
+      Ko->setIsKill(false);
+      // I suppose there should be only one operand with <kill> for UseR.
+      break;
----------------
Not necessarily.

Anyway, use MachineInstr::clearRegisterKills.

http://reviews.llvm.org/D8329

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/