[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/VLIWMachineScheduler.h:45
+  /// internal to the MI scheduler at the time.
+  std::vector<SUnit *> Packet;
+
----------------
MatzeB wrote:
> Isn't `SmallVector<>` typically the better choice over `std::vector`?
Correct: This is just what's upstream already.

I'm more than happy to make changes and clean the objects up once we determine that this patch is good.

The biggest gating issue right now is waiting to see if @kparzysz and team is able to apply and run their downstream Hexagon tests without seeing performance degradations.

Once that's cleared, I will clang-format and make changes like this in earnest to get it cleared for commit.


================
Comment at: llvm/lib/CodeGen/VLIWMachineScheduler.cpp:45
+
+static cl::opt<bool> IgnoreBBRegPressure("ignore-bb-reg-pressure",
+    cl::Hidden, cl::ZeroOrMore, cl::init(false));
----------------
MatzeB wrote:
> Maybe the options could be prefixed with `vliw-` or similar...
I was always hoping for some sort of option 'namespaces'.

Does cl::opt allow colons? Perhaps something like vliwmisched:<option> to denote pass-specific options.


================
Comment at: llvm/lib/CodeGen/VLIWMachineScheduler.cpp:104-107
+  case TargetOpcode::EXTRACT_SUBREG:
+  case TargetOpcode::INSERT_SUBREG:
+  case TargetOpcode::SUBREG_TO_REG:
+  case TargetOpcode::REG_SEQUENCE:
----------------
MatzeB wrote:
> Just for my curiosity: Are you really seeing those instructions? I thought they are used in early parts of the codegen pipeline but should be lowered away after `TwoAddressInstructionPass`...
I haven't done any investigation on my end. I don't think my downstream target utilizes them at the moment.

This is, however, Hexagon's original implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113150