[PATCH] D92068: [MachineCombiner] [NFC]Add MustReduceRegisterPressure goal

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

If this is NFC now, no objections from me - I just noted minor errors.
So we do not need LiveIntervals analysis at all now to reduce pressure, or that will be dealt with separately?



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1068
+
+  /// Return true if target supports to ressociate instrucitons in machine
+  /// combiner pass to reduce register pressure for a given BB.
----------------
typo: "if target supports reassociation of instructions"


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1076
+
+  /// fix up the placeholder we may add in genAlternativeCodeSequence()
+  virtual void
----------------
Make a complete sentence for code comments:
  /// Fix up the placeholder we may add in genAlternativeCodeSequence().


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1078
+  virtual void
+  finilizeInsInstrs(MachineInstr &Root, MachineCombinerPattern &P,
+                    SmallVectorImpl<MachineInstr *> &InsInstrs) const {}
----------------
typo: "finalize"


================
Comment at: llvm/lib/CodeGen/MachineCombiner.cpp:317
+  // We treat them as always profitable. But we can do better if we make
+  // RressureTracker class be aware of TIE attribute. Then we can get an
+  // accurate compare of register pressure with DelInstrs or InsInstrs.
----------------
typo: PressureTracker


================
Comment at: llvm/lib/CodeGen/MachineCombiner.cpp:478
+  // Otherwise the constant pool entry created for InsInstrs will not be deleted
+  // even InsInstrs is not the bettern pattern.
+  TII->finilizeInsInstrs(MI, Pattern, InsInstrs);
----------------
typo:
  // even if InsInstrs is not the better pattern.


================
Comment at: llvm/lib/CodeGen/MachineCombiner.cpp:636
+        if (MBB->size() > inc_threshold) {
+          // Use incremental depth updates for basic blocks above treshold
+          IncrementalUpdate = true;
----------------
typo (also in original code):
"threshold"


================
Comment at: llvm/lib/CodeGen/MachineCombiner.cpp:641
+        if (reduceRegisterPressure(MI, MBB, InsInstrs, DelInstrs, P)) {
+          // Replace DelInstrs with InInstrs.
+          insertDeleteInstructions(MBB, MI, InsInstrs, DelInstrs, MinInstr,
----------------
InInstrs -> InsInstrs


================
Comment at: llvm/lib/CodeGen/MachineCombiner.cpp:646
+
+          // back to previous instruction as it may have ILP reassociation
+          // opportunity.
----------------
back -> Go back ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92068/new/

https://reviews.llvm.org/D92068