[PATCH] D18427: CodeGen: Add DetectDeadLanes pass.

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Matthias,

This mostly looks good to me with a couple of nitpicks, remarks, and questions.

Cheers,
-Quentin


================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:25
@@ +24,3 @@
+///             use %vreg3
+/// The %vreg0 definition is dead and %vreg3 contains an undefined value.
+//
----------------
How do we end up generating this kind of code?

Although I agree we need to support it, we should try not to generate that.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:54
@@ +53,3 @@
+    LaneBitmask DefinedLanes;
+  };
+
----------------
Do not indent namespace:
http://llvm.org/docs/CodingStandards.html#namespace-indentation

I am surprise clang-format do not do the right thing.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:57
@@ +56,3 @@
+  class DetectDeadLanes : public MachineFunctionPass {
+    bool runOnMachineFunction(MachineFunction &MF) override;
+
----------------
Shouldn't this be public?

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:69
@@ +68,3 @@
+    }
+    void addUsedLanesOnOperand(const MachineOperand &MO, LaneBitmask UsedLanes);
+    void addDefinedLanesOnReg(unsigned Reg, LaneBitmask DefinedLanes);
----------------
Add doxygen comments.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:74
@@ +73,3 @@
+    void transferUsedLanesStep(const MachineOperand &Def,
+                               LaneBitmask UsedLanes);
+    /// Given a use regiser operand \p Use and a mask of defined lanes, check
----------------
Detail what does it mean to transfer the used lanes (BTW, you should use \p for UsedLanes).

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:90
@@ +89,3 @@
+    std::vector<VRegInfo> VRegInfos;
+    /// Worklist containins registers to process.
+    std::deque<unsigned> WorkList;
----------------
containing

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:92
@@ +91,3 @@
+    std::deque<unsigned> WorkList;
+    BitVector InWorkList;
+    /// This bitvector is set for each vreg index where the vreg is defined
----------------
Should we merge those two with like a SmallVectorSet?
Indeed, if I am readying the code correctly, both need to be in sync.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:97
@@ +96,3 @@
+  };
+}
+char DetectDeadLanes::ID = 0;
----------------
Override the getPassName method as well to ease debugging.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:108
@@ +107,3 @@
+  // are not lowered to a COPY.
+  return MI.isCopy() || MI.isPHI() || MI.isInsertSubreg() ||
+    MI.isRegSequence() || MI.isExtractSubreg();
----------------
A remark: since the goal right now is just to fix up the representation for the register coalescer, there is no value in supporting those.
However, if we go toward teaching this pass dead code elimination, then yes, it would make sense to do that.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:135
@@ +134,3 @@
+  if (DefinedByCopy[MORegIdx] && !InWorkList[MORegIdx])
+    addToWorklist(MORegIdx);
+}
----------------
I would have expected the check with InWorkList to be hidden into addToWorklist.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:141
@@ +140,3 @@
+  const MachineInstr &MI = *Def.getParent();
+  if (MI.isCopy() || MI.isPHI()) {
+    for (const MachineOperand &MO : MI.uses()) {
----------------
Use a switch on the Opcode.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:148
@@ +147,3 @@
+    unsigned I = MI.getNumOperands()-2;
+    for (;;) {
+      const MachineOperand &MO = MI.getOperand(I);
----------------
What is the benefit of going backward?
I found it confusing to have a "for(;;)" for this traversal.

I would have expected something like:
for (unsigned I = 1, E = MI.getNumOperands(); I != E; I += 2)

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:174
@@ +173,3 @@
+  } else {
+    assert(MI.isExtractSubreg());
+    const MachineOperand &MO = MI.getOperand(1);
----------------
Add a message in the assert. Like unexpected copy-like instruction.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:188
@@ +187,3 @@
+  const MachineInstr &MI = *Use.getParent();
+  if (MI.getDesc().getNumDefs() != 1)
+    return;
----------------
Add a comment why we limit ourselves to instruction with only one def.
For future reference, we need to know if this is a limitation or just that it does not make sense to support more than one definition.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:188
@@ +187,3 @@
+  const MachineInstr &MI = *Use.getParent();
+  if (MI.getDesc().getNumDefs() != 1)
+    return;
----------------
qcolombet wrote:
> Add a comment why we limit ourselves to instruction with only one def.
> For future reference, we need to know if this is a limitation or just that it does not make sense to support more than one definition.
What happens with inlineasm?
I.e., we do not get the actual number of definitions via the static description.
Explain why it is okay.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:211
@@ +210,3 @@
+  if (!InWorkList[DefRegIdx])
+    addToWorklist(DefRegIdx);
+}
----------------
Ditto: Check hidden inside addToWorklist.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:219
@@ +218,3 @@
+  // Translate DefinedLanes if necessary.
+  if (MI.isRegSequence()) {
+    unsigned SubIdx = MI.getOperand(OpNum+1).getImm();
----------------
switch for on the opcode

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:235
@@ +234,3 @@
+    unsigned SubIdx = MI.getOperand(2).getImm();
+    assert(OpNum == 1);
+    DefinedLanes
----------------
Message in assert.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:237
@@ +236,3 @@
+    DefinedLanes
+      = TRI->reverseComposeSubRegIndexLaneMask(SubIdx, DefinedLanes);
+  } else {
----------------
The formatting looks weird to me.
I.e., I would have expected clang-format to put the assignment operator on the previous line.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:252
@@ +251,3 @@
+    DEBUG(dbgs() << "Skipping Detect dead lanes pass\n");
+    return false;
+  }
----------------
The motivating example in the comment at the beginning of the file does not need sub register liveness to produce dead definitions after coalescing, does it?

I.e., is this true we do not need that pass when enableSubRegLiveness is false?

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:261
@@ +260,3 @@
+  InWorkList.resize(NumVirtRegs);
+  DefinedByCopy.resize(NumVirtRegs);
+
----------------
For the arrays, wouldn't using plain arrays be more effective?
In particular, we do not iterate on those and having easily access to there length is useless.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:305
@@ +304,3 @@
+        } else {
+          continue;
+        }
----------------
What does this else block covers?
A register is either virtual or physical, right?

What am I missing?

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:346
@@ +345,3 @@
+    WorkList.pop_front();
+    assert(InWorkList[RegIdx]);
+    InWorkList.reset(RegIdx);
----------------
Message in assert.

================
Comment at: lib/CodeGen/DetectDeadLanes.cpp:357
@@ +356,3 @@
+      transferDefinedLanesStep(MO, Info.DefinedLanes);
+    }
+  }
----------------
No need for the curly braces.


Repository:
  rL LLVM

http://reviews.llvm.org/D18427