Differential D95331
[MLIR][GPU][NVVM] Add conversion of warp synchronous matrix-multiply accumulate GPU ops Authored by navdeepkk on Jan 24 2021, 11:17 PM.
Details [MLIR][GPU][NVVM] Add conversion of warp synchronous matrix-multiply 1.) subgroup_mma_load_matrix -> wmma.m16n16k16.load.[a,b,c]..row.stride 2.) subgroup_mma_store_matrix -> wmma.m16n16k16.store.d.[f16,f32].row.stride 3.) subgroup_mma_compute -> wmma.m16n16k16.mma.row.row.[f16,f32].[f16,f32]
Diff Detail
Event TimelineComment Actions Changes in this diff :- 1.) Rebase on master to drop the use of LLVMType. 2.) Make changes to use the !gpu.mmafragment type introduced in parent revision D95330. Comment Actions I haven't done a detailed review since this commit may change if its parent changes.
Comment Actions Changes in this diff :- 1.) Rebase on upstream/main. 3.) Make changes to operate with the newly intoduced gpu.mma_matrix type.
Comment Actions This is okay with me except the splitting between files. I really don't understand the motivation behind adding header files to lib/. It is justified if there are some declarations or template private implementations that must be shared between several .cpp files and not visible to the external users, but here is is not the case. This can be organized as follows:
This is simple to navigate, reason about and is the pattern that all other conversions adopt.
Comment Actions Hi @ftynse,
Comment Actions You shouldn't even need to use them directly. Just declare a function populateWmmaToNVVMPatterns(RewritePatternSet &set) in GPUToNVVMPass.h, put its declaration in the header and implementation in WmmaOpsToNVVM.cpp. Then call this function from LowerGpuOpsToNVVMOps.cpp. You understand how function declaration works in C++, right? It's enough for the caller to see the declaration (not definition) to be able to call the function.
Absolutely not! It's almost never okay to include a .cpp into another .cpp.
You don't need to expose the classes, the function that adds instances of these classes into the set will amply suffice, as I mentioned repeatedly. If you look around a bit in the code base, LowerGpuOpsToNVVMOps.cpp defines the pattern classes in an anonymous namespace and they are not at all visible in the headers. The users of this lowering just call populateGpuToNVVMConversionPatterns and never ever need to see the classes. I see no reason why you can't just do the same. Comment Actions Thanks for the long series of great insightful comments. And apologies for taking too long in the re-structuring part. Comment Actions There appears to be something weird with this patch - doing an arc patch D95331 yields an exception and then shows zero changes. Looks like an arc bug: ...
Applied patch mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h cleanly.
Applied patch mlir/include/mlir/Dialect/LLVMIR/NVVMOps.td cleanly.
Applied patch mlir/include/mlir/Dialect/GPU/GPUOps.td cleanly.
Applied patch mlir/include/mlir/Dialect/GPU/GPUDialect.h cleanly.
Applied patch mlir/include/mlir/Dialect/GPU/GPUBase.td cleanly.
COMMITTED Successfully committed patch.
Cherry Pick Failed!
Exception
Command failed with error #1!
COMMAND
git cherry-pick -- arcpatch-D95330
STDOUT
Auto-merging mlir/test/Dialect/LLVMIR/invalid.mlir
Auto-merging mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
Auto-merging mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
Auto-merging mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
Auto-merging mlir/include/mlir/Dialect/GPU/GPUOps.td
On branch arcpatch-D95331_1
You are currently cherry-picking commit 372dcf47bd93.
(all conflicts fixed: run "git cherry-pick --continue")
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
Untracked files:
(use "git add <file>..." to include in what will be committed)
mlir/0001-MLIR-Update-affine.for-unroll-for-iter_args-support.patch
mlir/compile_commands.json
mlir/tags
mlir/tf.mlir
nothing added to commit but untracked files present (use "git add" to track)
STDERR
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:
git commit --allow-empty
Otherwise, please use 'git cherry-pick --skip'
(Run with `--trace` for a full exception trace.)Using --trace shows: Otherwise, please use 'git cherry-pick --skip' at [<arcanist>/src/future/exec/ExecFuture.php:421] arcanist(head=master, ref.master=239ad5c55d8d) #0 ExecFuture::raiseResultError(array) called at [<arcanist>/src/future/exec/ExecFuture.php:325] #1 ExecFuture::resolvex() called at [<arcanist>/src/repository/api/ArcanistRepositoryAPI.php:399] #2 ArcanistRepositoryAPI::execxLocal(string, string) called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:778] #3 ArcanistPatchWorkflow::run() called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:983] #4 ArcanistPatchWorkflow::applyDependencies(ArcanistBundle) called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:469] #5 ArcanistPatchWorkflow::run() called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:398] #6 ArcanistPatchWorkflow::run() called at [<arcanist>/scripts/arcanist.php:419] <<< [1] (+15,126) <exec> 15,126,163 us | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
C++ style comment here. Terminate with full stop.