[Machine Combiner] Valid use of OptSize

[avt77]

Summary

Currently we're able to select longer alternative code sequence if it has better sched numbers but we should always select the shortest code sequence if OptSize == true. This patch fixes the given issue.

[RKSimon]

test case?

[avt77]

In D43813#1020647, @RKSimon wrote:

test case?

Unfortunately, before D40602 (or D26855 which I'm going to update soon) we don't have any test case because we don't use Machine Combiner when alternative sequence of instructions is longer then the original one.
But D40602 has such test just now (see lines 420-423 in test/CodeGen/X86/schedule-x86-64-shld.ll): at the moment we can select longer code sequence even if we have optsize option for the function (see define void @lshift_mem_b_optsize(i64 %b) nounwind readnone optsize {).

[avt77]

Ping!
Gerolf, could you give me LGTM for this tiny patch?

[RKSimon]

LGTM with a couple of style comments - pity we can't get a test case

lib/CodeGen/MachineCombiner.cpp

413

return (NewSize < OldSize);  // Only substitute if new size < old size

594

} else if (!OptSize || (NewInstCount <= OldInstCount))) {

[avt77]

In D43813#1040151, @RKSimon wrote:

LGTM with a couple of style comments - pity we can't get a test case

Could you press "Accepted"?

lib/CodeGen/MachineCombiner.cpp

DEBUG(
                       "Length\n");
 
   return ResLenAfterCombine <= ResLenBeforeCombine;
 }
 
 /// \returns true when new instruction sequence should be generated
 /// independent if it lengthens critical path or not
 bool MachineCombiner::doSubstitute(unsigned NewSize, unsigned OldSize) {
-  if (OptSize && (NewSize < OldSize))
-    return true;
+  if (OptSize) { // First of all check OptSize option
+    if (NewSize < OldSize)
+      return true; // Do substitute in any case if new size < old size
+    return false;  // We should not do substitute if new size >= old size
+  }
   if (!TSchedModel.hasInstrSchedModelOrItineraries())
     return true;
   return false;
 }
 
 /// Inserts InsInstrs and deletes DelInstrs. Incrementally updates instruction
 /// depths if requested.
 ///
for (auto P : Patterns) {
       // the new sequence neither lengthens the critical path nor increases
       // resource pressure.
       if (SubstituteAlways || doSubstitute(NewInstCount, OldInstCount)) {
         insertDeleteInstructions(MBB, MI, InsInstrs, DelInstrs, MinInstr,
                                  RegUnits, IncrementalUpdate);
         // Eagerly stop after the first pattern fires.
         Changed = true;
         break;
-      } else {
+      } else if (!(OptSize && (NewInstCount > OldInstCount))) {
         // For big basic blocks, we only compute the full trace the first time
         // we hit this. We do not invalidate the trace, but instead update the
         // instruction depths incrementally.
         // NOTE: Only the instruction depths up to MI are accurate. All other
         // trace information is not updated.
         MachineTraceMetrics::Trace BlockTrace = MinInstr->getTrace(MBB);
         Traces->verifyAnalysis();
         if (improvesCriticalPathLen(MBB, &MI, BlockTrace, InsInstrs, DelInstrs,