[PATCH] D69998: [MacroFusion] Create the missing artificial edges if there are more than 2 SU fused.

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

My understanding is that you are doing two fixes in this patch:

1. Extend API of `shouldScheduleAdjacent` to `avoid fusing an instruction more than once along the same dependency chain`.
2. Extend `fuseInstructionPair` to add artificial data dependencies for chained-fusion

To me, yes, we should do #1 first, as https://reviews.llvm.org/D36704 was already trying to do so.

#2 might not be necessary, as you mentioned, all existing targets only support `back-to-back` fusion.
If we want to extend it for `chained-fusion` (3 or more in same dependency chain),
then I believe we need more changes than adding data dependencies.
Also need additional tests for some target that support it (maybe RISC-V?)



================
Comment at: llvm/include/llvm/CodeGen/MacroFusion.h:34
+                                                const MachineInstr &SecondMI,
+                                                unsigned NumFused)>;
 
----------------
New parameters, should be documented in above comment as well.


================
Comment at: llvm/test/CodeGen/AArch64/macro-fusion-verify.ll:4
+
+; Verify that, the macro-fusion won't bring in extra dependency.
+define signext i32 @test(i32 signext %a, i32 signext %b, i32 signext %c, i32 signext %d) {
----------------
This comment is confusing. I believe the goal of this patch is to avoid chained-fusion, hence reducing unnecessary dependency, so you would like to verify that there is no `extra dependency`?


================
Comment at: llvm/test/CodeGen/AArch64/macro-fusion-verify.ll:25
+; CHECK: Successors:
+; CHECK: SU([[SU5]]): Ord  Latency=0 Cluster
+
----------------
Maybe we should check that we only fuse `SU4 SU5`, not `SU5 SU6` too.


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

https://reviews.llvm.org/D69998