[PATCH] D15667: [MachineScheduler] Handle regmasks and allow calls to be rescheduled.

[Andrew Trick via llvm-commits]

atrick added a comment.

High level, I have the same questions as Hal. But I'm not opposed to adding the support so that targets can evaluate the possibility of call scheduling.

Please test correctness at least on x86 by running the InstructionShuffler with RescheduleCalls.


================
Comment at: lib/CodeGen/MachineScheduler.cpp:390-399
@@ -391,3 +389,12 @@
                             const TargetInstrInfo *TII) {
-  return MI->isCall() || TII->isSchedulingBoundary(MI, MBB, *MF);
+  // Calls may be rescheduled if demanded by CL option or by
+  // subtarget.
+  if (MI->isCall() && !MI->isTerminator()) {
+    if (RescheduleCalls.getNumOccurrences())
+      return !RescheduleCalls;
+    else
+      return (!MF->getSubtarget().MISchedRescheduleCalls());
+  }
+
+  return TII->isSchedulingBoundary(MI, MBB, *MF);
 }
----------------
Don't we still want to call TII->isSchedulingBoundary for calls? The target may treat some calls differently. How about this:

Always honor the command line option if present (it's just for debugging/experimentation anyway):

  if (RescheduleCalls.getNumOccurrences())
      return !RescheduleCalls;

Otherwise simply defer to the target:

  return TII->isSchedulingBoundary(MI, MBB, *MF);

By default, have TargetInstrInfo::isSchedulingBoundary return true for isCall().

Make sure any in-tree targets that override this method return true for isCall().

Post on llvm-dev explaining that the scheduler now works across calls. If you have an out-of-tree target that overrides 
TargetInstrInfo::isSchedulingBoundary, you now need to check isCall yourself.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:296-301
@@ +295,8 @@
+  Reg2SUnitsMap::iterator I = P.second;
+  for (bool isBegin = I == B; !isBegin; /* empty */) {
+    isBegin = (--I) == B;
+    if (!I->SU->isCall)
+      break;
+    I = Defs.erase(I);
+  }
+}
----------------
I know this is copied code, but I don't understand why you break out of the loop here.

Also, I don't understand why you don't just clear all calls in your case, since you're about to add the most call to the Defs set.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:316-317
@@ +315,4 @@
+      for (MCRegAliasIterator Alias(reg, TRI, true); Alias.isValid(); ++Alias) {
+        if (!Defs.contains(*Alias))
+          continue;
+        for (Reg2SUnitsMap::iterator I = Defs.find(*Alias); I != Defs.end(); ++I) {
----------------
I think this is a redundant check (as it is in addPhysRegDataDeps).



http://reviews.llvm.org/D15667