[PATCH] D69998: [MacroFusion] Create the missing artificial edges if there are more than 2 SU fused.
Jinsong Ji via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 8 20:28:12 PST 2019
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
More information about the llvm-commits
mailing list