MachineCombiner: consider all uses in getLatency()

[artagnon]

Summary

getLatency() was considering only the first use when computing operand
latency. Fix this.

[barannikov88]

llvm/lib/CodeGen/MachineCombiner.cpp

224

This change is not correct. MI->uses() is misnomer, it returns everything apart explicit defs. I.e. it returns explicit uses, implicit uses, and implicit defs.

279

This, too. MI->defs()` returns only explicit defs, it does not include implicit ones.

[barannikov88]

This will still work because virtual registers cannot be implicit operands, and there is a check for virtualness above.
Still, using defs and uses should be avoided because they have unexpected behavior.
I hope they can be removed someday and then reimplemented correctly w.r.t. implicit operands.

[artagnon]

Address review by barannikov88

[barannikov88]

Shouldn't there be any test changes?
I'm not familiar with this pass and can't help with further review.

llvm/lib/CodeGen/MachineCombiner.cpp

286

(Assuming iterating over all uses is necessary.)
If the register is used by a single instruction multiple times, findRegisterUseOperandIdx will always find the first use.
I think UseMO : MRI->use_[nodbg_]operands(MO.getReg()) should be used instead in the for loop, and UseMO.getOperandNo() here.

[artagnon]

Address review comments. There are no test changes, but Embench shows a minor improvement in one test.

[barannikov88]

In D150855#4352644, @artagnon wrote:

There are no test changes, but Embench shows a minor improvement in one test.

Can this test be reduced and added to the review? I'm afraid this can't proceed without test coverage.

llvm/lib/CodeGen/MachineCombiner.cpp

286

This now gets the operand number of the defining instruction. We need the operand number of the using instruction here; it is likely to be different.
Please consider using use_nodbg_operands. You can get the UseInstr from it by calling UseMO.getParent().

[barannikov88]

llvm/lib/CodeGen/MachineCombiner.cpp

286

(FWIW NewRoot->findRegisterDefOperandIdx(MO.getReg()) could probably also be simplified to MO.getOperandNo(), but I'm sure. Perhaps there was a reason for not doing so. This is not a change request.)

286

but I'm sure
but I'm not sure

[artagnon]

In D150855#4352664, @barannikov88 wrote:

In D150855#4352644, @artagnon wrote:

There are no test changes, but Embench shows a minor improvement in one test.

Can this test be reduced and added to the review? I'm afraid this can't proceed without test coverage.

I just re-verified, and there are no benchmark changes either. I'm not sure how to cook up a test for this change.

llvm/lib/CodeGen/MachineCombiner.cpp

MachineCombiner::getDepth(SmallVectorImpl<MachineInstr *> &InsInstrs,
   // operands. Use the trace information when possible. For new operands which
   // are tracked in the InstrIdxForVirtReg map depth is looked up in InstrDepth
   for (auto *InstrPtr : InsInstrs) { // for each Use
     unsigned IDepth = 0;
     for (const MachineOperand &MO : InstrPtr->operands()) {
       // Check for virtual register operand.
       if (!(MO.isReg() && MO.getReg().isVirtual()))
         continue;
       if (!MO.isUse())
         continue;
       unsigned DepthOp = 0;
       unsigned LatencyOp = 0;
       DenseMap<unsigned, unsigned>::iterator II =
           InstrIdxForVirtReg.find(MO.getReg());
       if (II != InstrIdxForVirtReg.end()) {
         // Operand is new virtual register not in trace
         assert(II->second < InstrDepth.size() && "Bad Index");
unsigned MachineCombiner::getLatency(MachineInstr *Root, MachineInstr *NewRoot,
                                      MachineTraceMetrics::Trace BlockTrace) {
   // Check each definition in NewRoot and compute the latency
   unsigned NewRootLatency = 0;
 
   for (const MachineOperand &MO : NewRoot->operands()) {
     // Check for virtual register operand.
     if (!(MO.isReg() && MO.getReg().isVirtual()))
       continue;
     if (!MO.isDef())
       continue;
-    // Get the first instruction that uses MO
-    MachineRegisterInfo::reg_iterator RI = MRI->reg_begin(MO.getReg());
-    RI++;
-    if (RI == MRI->reg_end())
-      continue;
-    MachineInstr *UseMO = RI->getParent();
+    for (MachineInstr &UseInstr : MRI->use_nodbg_instructions(MO.getReg())) {
       unsigned LatencyOp = 0;
-    if (UseMO && BlockTrace.isDepInTrace(*Root, *UseMO)) {
+      if (BlockTrace.isDepInTrace(*Root, UseInstr)) {
         LatencyOp = TSchedModel.computeOperandLatency(
-          NewRoot, NewRoot->findRegisterDefOperandIdx(MO.getReg()), UseMO,
-          UseMO->findRegisterUseOperandIdx(MO.getReg()));
+            NewRoot, NewRoot->findRegisterDefOperandIdx(MO.getReg()), &UseInstr,
+            MO.getOperandNo());
       } else {
         LatencyOp = TSchedModel.computeInstrLatency(NewRoot);
       }
       NewRootLatency = std::max(NewRootLatency, LatencyOp);
     }
+  }
   return NewRootLatency;
 }
 
 /// The combiner's goal may differ based on which pattern it is attempting
 /// to optimize.
 enum class CombinerObjective {
   MustReduceDepth,            // The data dependency chain must be improved.
   MustReduceRegisterPressure, // The register pressure must be reduced.