[PATCH] D113150: Lift VLIWResourceModel, VLIWMachineScheduler, and ConvergingVLIWScheduler into CodeGen/VLIWMachineScheduler

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:104
 extern cl::opt<bool> VerifyScheduling;
+#ifndef NDEBUG
+extern cl::opt<bool> ViewMISchedDAGs;
----------------
I really wasn't sure if this was the best way to hoist this option. Something similar is done in other files, but the NDEBUG switch between cl::opt<bool> and bool makes me a little bit uncomfortable.

The other option is to instead put this block into VLIWMachineScheduler, but I feel that too has problems, since it's conceivable that any file which includes MachineScheduler may want access to these options.


================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:50
 public:
-  VLIWResourceModel(const TargetSubtargetInfo &STI, const TargetSchedModel *SM)
-      : TII(STI.getInstrInfo()), SchedModel(SM) {
-    ResourcesModel = createPacketizer(STI);
+  VLIWResourceModel(const TargetSubtargetInfo &STI, const TargetSchedModel *SM);
 
----------------
A number of header implementations have been moved to the source file to allow the header to be more independent of header inclusion, This is an example.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:94
+#ifndef NDEBUG
+cl::opt<bool> ViewMISchedDAGs(
+    "view-misched-dags", cl::Hidden,
----------------
These ended up being moved into the llvm namespace. All but 1 example of options declared in the header were placed into the namespace, and doing so made intuitive sense to me so that the global namespace remains relatively clean.


================
Comment at: llvm/lib/CodeGen/VLIWMachineScheduler.cpp:45
 
-static cl::opt<bool> IgnoreBBRegPressure("ignore-bb-reg-pressure",
-    cl::Hidden, cl::ZeroOrMore, cl::init(false));
+static cl::opt<bool> IgnoreBBRegPressure("ignore-bb-reg-pressure", cl::Hidden,
+                                         cl::ZeroOrMore, cl::init(false));
----------------
I forgot this comment...

I'll prefix generic-looking options with vliw-misched, since that is a natural prefix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113150