[PATCH] D73643: [RISCV] Macro Fusion for RISC-V

[James Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

Indexed load and store fusion should not be exclusive to `LD` and `SD`. It applies to any `(F)Lx`/`(F)Sx`. The literature just talks about `LD`/`ST` (note that this is not `SD`) in the sense of an arbitrary load/store instruction. Similarly for the various other fusion pairs involving memory accesses.



================
Comment at: llvm/lib/Target/RISCV/RISCV.td:71
+def FeatureMacroFusion
+    : SubtargetFeature<"macrofusion", "HasMacroFusion", "true",
+                "Various pair of instructions can be fused">;
----------------
evandro wrote:
> Different implementations may implement just some of the possible pairs.  So this feature should perhaps be broken down into each category.
This is a multi-word feature, so should be hyphenated as `macro-fusion` in my opinion (although I would prefer `macro-op-fusion` over that...).


================
Comment at: llvm/lib/Target/RISCV/RISCVMacroFusion.cpp:37
+        // Far Jumps
+        case RISCV::LD:
+          return true;
----------------
This is clearly not a jump.


================
Comment at: llvm/lib/Target/RISCV/RISCVMacroFusion.cpp:61
+// Fuse Load Immediate Idioms
+static bool isClearUpWord(const MachineInstr *FirstMI,
+                              const MachineInstr &SecondMI) {
----------------
This is not the right name. This is for load immediate idioms.


================
Comment at: llvm/lib/Target/RISCV/RISCVMacroFusion.cpp:79
+                                  const MachineInstr &SecondMI) {
+  return FirstMI->getOpcode() == RISCV::LD &&
+          SecondMI.getOpcode() == RISCV::LD;
----------------
We don't want to be fusing arbitrary loads/stores together, only those we know to be to adjacent memory locations with the same width (and in that order). Moreover it can only be done in decode if it's trivial to determine these conditions hold, ie they use the same opcode and base register, differing only in the immediate by the memory access width.


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

https://reviews.llvm.org/D73643