[PATCH] D73643: Macro Fusion for RISC-V

[Evandro Menezes via Phabricator via llvm-commits]

evandro added a comment.

Please, prefix the title with [RISCV].



================
Comment at: llvm/lib/Target/RISCV/RISCV.td:71
+def FeatureMacroFusion
+    : SubtargetFeature<"macrofusion", "HasMacroFusion", "true",
+                "Various pair of instructions can be fused">;
----------------
Different implementations may implement just some of the possible pairs.  So this feature should perhaps be broken down into each category.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:72
+    : SubtargetFeature<"macrofusion", "HasMacroFusion", "true",
+                "Various pair of instructions can be fused">;
+
----------------
"Fuse select instruction pairs"


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:119
 def : ProcessorModel<"rocket-rv64", Rocket64Model, [Feature64Bit,
-                     FeatureRVCHints]>;
+                     FeatureRVCHints, FeatureMacroFusion]>;
 
----------------
Does Rocket fuse all these pairs?


================
Comment at: llvm/lib/Target/RISCV/RISCVMacroFusion.cpp:102
+  // We're only checking whether Second can be fused at all.
+  if (FirstMI == nullptr)
+    return true;
----------------
This is not correct as it increases compile time even when `SecondMI` is not a candidate for a fuseable pair.  `MacroFusion::scheduleAdjacentImpl()` will call this function to query if `SecondMI` before traversing its predecessors.  So you should go into the functions below and they should handle the case for `FirstMI` being `nullptr` for this test.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:131
+  }
+  ScheduleDAGInstrs *
+  createPostMachineScheduler(MachineSchedContext *C) const override {
----------------
Please, insert a blank line here.


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

https://reviews.llvm.org/D73643