[PATCH] D132079: [AMDGPU] Add iglp_opt builtin and MFMA GEMM Opt strategy
jrbyrnes accepted this revision. jrbyrnes added a comment. This revision is now accepted and ready to land. LGTM Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:427 DAG->addMutation(createStoreClusterDAGMutation(DAG->TII, DAG->TRI)); DAG->addMutation(createIGroupLPDAGMutation()); DAG->addMutation(createAMDGPUMacroFusionDAGMutation()); kerbowa wrote: > jrbyrnes wrote: > > I think you can remove this as well since you're doing it from within the > > scheduler. > It's not added in the scheduler for plain SCHED_BARRIER. Oh okay -- I see Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132079/new/ https://reviews.llvm.org/D132079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132079: [AMDGPU] Add iglp_opt builtin and MFMA GEMM Opt strategy
kerbowa added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:427 DAG->addMutation(createStoreClusterDAGMutation(DAG->TII, DAG->TRI)); DAG->addMutation(createIGroupLPDAGMutation()); DAG->addMutation(createAMDGPUMacroFusionDAGMutation()); jrbyrnes wrote: > I think you can remove this as well since you're doing it from within the > scheduler. It's not added in the scheduler for plain SCHED_BARRIER. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132079/new/ https://reviews.llvm.org/D132079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132079: [AMDGPU] Add iglp_opt builtin and MFMA GEMM Opt strategy
jrbyrnes added a comment. Just a couple nitpicks Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1071 PipelineSolver PS(SyncedSchedGroups, SyncedInstrs, DAG); // PipelineSolver performs the mutation by adding the edges it Have a fully unguarded entry point into PS construction / PS.solve() makes me a bit uneasy -- and it is at best inefficient. Can you guard this with foundSGB || foundIGLP? Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:427 DAG->addMutation(createStoreClusterDAGMutation(DAG->TII, DAG->TRI)); DAG->addMutation(createIGroupLPDAGMutation()); DAG->addMutation(createAMDGPUMacroFusionDAGMutation()); I think you can remove this as well since you're doing it from within the scheduler. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132079/new/ https://reviews.llvm.org/D132079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132079: [AMDGPU] Add iglp_opt builtin and MFMA GEMM Opt strategy
jrbyrnes added a comment. LGTM again Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132079/new/ https://reviews.llvm.org/D132079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132079: [AMDGPU] Add iglp_opt builtin and MFMA GEMM Opt strategy
jrbyrnes added inline comments.
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1063
+} else if (Opc == AMDGPU::IGLP_OPT) {
+ if (!foundSB && !foundIGLP)
+initIGLPOpt(*R);
I think this makes more sense if you parse the entire dag first, then check if
neither were found.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132079/new/
https://reviews.llvm.org/D132079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132079: [AMDGPU] Add iglp_opt builtin and MFMA GEMM Opt strategy
jrbyrnes added a comment.
Hey Austin --
Just have a small question about the purpose of shouldApplyStrategy -- other
than that, LGTM.
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:758
+
+ bool shouldApplyStrategy(ScheduleDAGInstrs *DAG) override { return true; }
+
Is the plan to use heuristics on top of the builtin at some point? Not sure I
understand this.
Comment at: llvm/lib/Target/AMDGPU/SIPostRABundler.cpp:135
+// Don't cluster with IGLP instructions.
+bool HasIGLPInstrs =
+std::any_of(MBB.instr_begin(), MBB.instr_end(), [](MachineInstr ) {
Maybe not in this patch due to time constraints, but perhaps in future work we
can extract checking for IGLP_OPT / SCHED_GROUP_BARRIER to an analysis patch so
we don't need to keep checking for it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132079/new/
https://reviews.llvm.org/D132079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
