[PATCH] D40602: [X86] Add MC level selection support for SHLD (64-bit only)

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added inline comments.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:189
         int UseIdx = InstrPtr->findRegisterUseOperandIdx(MO.getReg());
-        LatencyOp = TSchedModel.computeOperandLatency(DefInstr, DefIdx,
-                                                      InstrPtr, UseIdx);
+        if (DefIdx < 0 || UseIdx < 0)
+          // W/o def/use indexes we can't compute latency based on shed model
----------------
Shouldn't a case like this be handled by computeOperandLatency?


================
Comment at: lib/CodeGen/MachineCombiner.cpp:418
     return true;
+  if (OptSize && (NewSize > OldSize))
+    return false;
----------------
What is the point? Now the return values for if conditions are in-consistent and I find this harder to read. 


================
Comment at: lib/CodeGen/MachineCombiner.cpp:570
           dbgs() << "\t\t" << STI->getSchedInfoStr(*InstrPtr) << ": ";
-          InstrPtr->print(dbgs(), false, false, TII);
-          dbgs() << "\n";
+          InstrPtr->print(dbgs(), false, false, false, TII);
         }
----------------
Why needed for this commit?


================
Comment at: lib/CodeGen/MachineCombiner.cpp:599
         break;
-      } else {
+      } else if (!(OptSize && (NewInstCount > OldInstCount))) {
         // For big basic blocks, we only compute the full trace the first time
----------------
Could you commit the size checks separately?



================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3809
   }
+  MCShldEnabled =
+      MCShldEnabled & Subtarget.getSchedModel().hasInstrSchedModel();
----------------
Should the be part of isMCShldEnabled?


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:11617
+  // SHLD patterns
+  if (MCShldEnabled && getSHLDPatterns(Root, Patterns))
+    return true;
----------------
IsMCShldEnabled?



================
Comment at: test/CodeGen/X86/schedule-x86-64-shld.ll:1
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mc-shld-enabled -print-schedule -mcpu=x86-64 | FileCheck %s --check-prefix=CHECK --check-prefix=GENERIC
----------------
Why do you need to change these test cases?


https://reviews.llvm.org/D40602