Skip to content

[mlir][gpu] Refactor GpuOpsToROCDLOps pass interface (NFC) #157402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 10, 2025
Merged

[mlir][gpu] Refactor GpuOpsToROCDLOps pass interface (NFC) #157402

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c03637b
[mlir][gpu] GPUToROCDL: Add C++ argument to populate allowedDialects
pabloantoniom Sep 8, 2025
5572996
[Refactor] Force users to use create* function autogenerated from tab…
pabloantoniom Sep 8, 2025
5dbb951
Cleanup GPUToROCDLPass header
pabloantoniom Sep 9, 2025
a118652
Delete constructors and use the ones from base
pabloantoniom Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion mlir/include/mlir/Conversion/GPUToROCDL/GPUToROCDLPass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

#include "mlir/Conversion/GPUToROCDL/Runtimes.h"
#include "mlir/Conversion/LLVMCommon/LoweringOptions.h"
#include "llvm/ADT/DenseSet.h"
#include <cstddef>
Copy link
Collaborator

@joker-eph joker-eph Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you're only removing something from the header, so I'm not sure why you need to add new includes?

Copy link
Contributor Author

@pabloantoniom pabloantoniom Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot to remove those after deleting the constructor in the .td. Only Pass is needed, since now TableGen is creating the create* function which uses mlir::Pass, but I have removed the include and added Pass to the namespace below instead.

#include <memory>

namespace mlir {
Expand Down Expand Up @@ -50,7 +52,9 @@ createLowerGpuOpsToROCDLOpsPass(
const std::string &chipset = "gfx900",
unsigned indexBitwidth = kDeriveIndexBitwidthFromDataLayout,
bool useBarePtrCallConv = false,
gpu::amd::Runtime runtime = gpu::amd::Runtime::Unknown);
gpu::amd::Runtime runtime = gpu::amd::Runtime::Unknown,
const std::optional<llvm::SmallDenseSet<llvm::StringRef>> &allowedDialects =
std::nullopt);
Copy link
Collaborator

@joker-eph joker-eph Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TableGen already generates all the suitable creation function, we should be able to remove this entirely and use the generated one instead (what is missing?)

Copy link
Contributor Author

@pabloantoniom pabloantoniom Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TableGen already generates all the suitable creation function, we should be able to remove this entirely and use the generated one instead (what is missing?)

I'm not sure I understand your suggestion, sorry. If you try to call createLowerGpuOpsToROCDLOpsPass (i.e., from the pass manager) in C++, prior to this PR, there was no way to specify allowedDialects, so this PR adds support for that (Unless I'm missing some TableGen functionality that I did not know about)

Copy link
Contributor

@fabianmcg fabianmcg Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mehdi is suggesting that you remove https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Conversion/Passes.td#L627 and let tablegen generate the create* calls.

Copy link
Contributor Author

@pabloantoniom pabloantoniom Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I understood that tablegen was supposed to generate the same create* function as the handwritten one, thanks @fabianmcg for the clarification. I have pushed a commit to address this.


} // namespace mlir

Expand Down
23 changes: 15 additions & 8 deletions mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,10 @@ struct GPUShuffleOpLowering : public ConvertOpToLLVMPattern<gpu::ShuffleOp> {
struct LowerGpuOpsToROCDLOpsPass final
: public impl::ConvertGpuOpsToROCDLOpsBase<LowerGpuOpsToROCDLOpsPass> {
LowerGpuOpsToROCDLOpsPass() = default;
LowerGpuOpsToROCDLOpsPass(const std::string &chipset, unsigned indexBitwidth,
bool useBarePtrCallConv,
gpu::amd::Runtime runtime) {
LowerGpuOpsToROCDLOpsPass(
const std::string &chipset, unsigned indexBitwidth,
bool useBarePtrCallConv, gpu::amd::Runtime runtime,
std::optional<llvm::SmallDenseSet<StringRef>> allowedDialects) {
if (this->chipset.getNumOccurrences() == 0)
this->chipset = chipset;
if (this->indexBitwidth.getNumOccurrences() == 0)
Expand All @@ -299,6 +300,12 @@ struct LowerGpuOpsToROCDLOpsPass final
this->useBarePtrCallConv = useBarePtrCallConv;
if (this->runtime.getNumOccurrences() == 0)
this->runtime = runtime;
if (this->allowedDialects.getNumOccurrences() == 0 &&
allowedDialects.has_value()) {
for (auto &str : allowedDialects.value()) {
this->allowedDialects.push_back(str.str());
}
}
}

void getDependentDialects(DialectRegistry &registry) const override {
Expand Down Expand Up @@ -501,10 +508,10 @@ void mlir::populateGpuToROCDLConversionPatterns(
}

std::unique_ptr<OperationPass<gpu::GPUModuleOp>>
mlir::createLowerGpuOpsToROCDLOpsPass(const std::string &chipset,
unsigned indexBitwidth,
bool useBarePtrCallConv,
gpu::amd::Runtime runtime) {
mlir::createLowerGpuOpsToROCDLOpsPass(
const std::string &chipset, unsigned indexBitwidth, bool useBarePtrCallConv,
gpu::amd::Runtime runtime,
const std::optional<llvm::SmallDenseSet<StringRef>> &allowedDialects) {
return std::make_unique<LowerGpuOpsToROCDLOpsPass>(
chipset, indexBitwidth, useBarePtrCallConv, runtime);
chipset, indexBitwidth, useBarePtrCallConv, runtime, allowedDialects);
}