This is an archive of the discontinued LLVM Phabricator instance.

Differential D91252

MLIR: add SinOp Lowering to __ocml_sin_f32 and __ocml_sin_f64
ClosedPublic

Authored by akuegel on Nov 11 2020, 5:18 AM.

Details

Reviewers
herhut
ftynse
Commits
rG5248047c9396: MLIR: add SinOp Lowering to __ocml_sin_f32 and __ocml_sin_f64
Summary

This mimics the recent similar patch for GPUToNVVM.

Diff Detail

Event Timeline

akuegel created this revision.Nov 11 2020, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2020, 5:18 AM
Herald added subscribers: rdzhabarov, tatianashp, msifontes and 14 others. · View Herald Transcript
akuegel requested review of this revision.Nov 11 2020, 5:18 AM
Herald added a reviewer: herhut. · View Herald TranscriptNov 11 2020, 5:18 AM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
akuegel added a reviewer: ftynse.Nov 11 2020, 5:22 AM
herhut added a comment.Nov 11 2020, 5:23 AM

Thanks for fixing this!

Can you use [mlir][gpu] in the subject and make it clear that this applies to the rocm lowering only?

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp
75

Just wondering, why is there no TanOp here? Could you add this, as well?

118

Can you keep these sorted?

akuegel updated this revision to Diff 304490.Nov 11 2020, 5:29 AM

Address review comments.

akuegel marked an inline comment as done.Nov 11 2020, 5:30 AM

Thanks for the review comments :)

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp
75

Yes, looks like this was an oversight. I have added it now as well.

herhut accepted this revision.Nov 11 2020, 5:32 AM

Thank you!

This revision is now accepted and ready to land.Nov 11 2020, 5:32 AM
Harbormaster completed remote builds in B78447: Diff 304489.Nov 11 2020, 5:35 AM
This revision was landed with ongoing or failed builds.Nov 11 2020, 5:39 AM
Closed by commit rG5248047c9396: MLIR: add SinOp Lowering to __ocml_sin_f32 and __ocml_sin_f64 (authored by akuegel). · Explain Why
This revision was automatically updated to reflect the committed changes.
akuegel added a commit: rG5248047c9396: MLIR: add SinOp Lowering to __ocml_sin_f32 and __ocml_sin_f64.
Harbormaster completed remote builds in B78448: Diff 304490.Nov 11 2020, 5:45 AM

Revision Contents

PathSize
mlir/
lib/
Conversion/
GPUToROCDL/
4 lines

Diff 304489

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

Show First 20 Lines • Show All 66 Lines • ▼ Show 20 Lines void runOnOperation() override {
populateVectorToLLVMConversionPatterns(converter, llvmPatterns); populateVectorToLLVMConversionPatterns(converter, llvmPatterns);
populateVectorToROCDLConversionPatterns(converter, llvmPatterns); populateVectorToROCDLConversionPatterns(converter, llvmPatterns);
populateStdToLLVMConversionPatterns(converter, llvmPatterns); populateStdToLLVMConversionPatterns(converter, llvmPatterns);
populateGpuToROCDLConversionPatterns(converter, llvmPatterns); populateGpuToROCDLConversionPatterns(converter, llvmPatterns);
LLVMConversionTarget target(getContext()); LLVMConversionTarget target(getContext());
target.addIllegalDialect<gpu::GPUDialect>(); target.addIllegalDialect<gpu::GPUDialect>();
target.addIllegalOp<LLVM::CosOp, LLVM::ExpOp, LLVM::FAbsOp, LLVM::FCeilOp, target.addIllegalOp<LLVM::CosOp, LLVM::ExpOp, LLVM::FAbsOp, LLVM::FCeilOp,
LLVM::FFloorOp, LLVM::LogOp, LLVM::Log10Op, LLVM::FFloorOp, LLVM::LogOp, LLVM::Log10Op,
LLVM::Log2Op>(); LLVM::Log2Op, LLVM::SinOp>();
herhutUnsubmitted
Not Done
ReplyInline Actions

Just wondering, why is there no TanOp here? Could you add this, as well?

herhut: Just wondering, why is there no `TanOp` here? Could you add this, as well?
akuegelAuthorUnsubmitted
Done
ReplyInline Actions

Yes, looks like this was an oversight. I have added it now as well.

akuegel: Yes, looks like this was an oversight. I have added it now as well.
target.addIllegalOp<FuncOp>(); target.addIllegalOp<FuncOp>();
target.addLegalDialect<ROCDL::ROCDLDialect>(); target.addLegalDialect<ROCDL::ROCDLDialect>();
// TODO: Remove once we support replacing non-root ops. // TODO: Remove once we support replacing non-root ops.
target.addLegalOp<gpu::YieldOp, gpu::GPUModuleOp, gpu::ModuleEndOp>(); target.addLegalOp<gpu::YieldOp, gpu::GPUModuleOp, gpu::ModuleEndOp>();
if (failed(applyPartialConversion(m, target, std::move(llvmPatterns)))) if (failed(applyPartialConversion(m, target, std::move(llvmPatterns))))
signalPassFailure(); signalPassFailure();
} }
}; };
Show All 26 Linesvoid mlir::populateGpuToROCDLConversionPatterns(
patterns.insert<OpToFuncCallLowering<LogOp>>(converter, "__ocml_log_f32", patterns.insert<OpToFuncCallLowering<LogOp>>(converter, "__ocml_log_f32",
"__ocml_log_f64"); "__ocml_log_f64");
patterns.insert<OpToFuncCallLowering<Log10Op>>(converter, "__ocml_log10_f32", patterns.insert<OpToFuncCallLowering<Log10Op>>(converter, "__ocml_log10_f32",
"__ocml_log10_f64"); "__ocml_log10_f64");
patterns.insert<OpToFuncCallLowering<Log2Op>>(converter, "__ocml_log2_f32", patterns.insert<OpToFuncCallLowering<Log2Op>>(converter, "__ocml_log2_f32",
"__ocml_log2_f64"); "__ocml_log2_f64");
patterns.insert<OpToFuncCallLowering<TanhOp>>(converter, "__ocml_tanh_f32", patterns.insert<OpToFuncCallLowering<TanhOp>>(converter, "__ocml_tanh_f32",
"__ocml_tanh_f64"); "__ocml_tanh_f64");
patterns.insert<OpToFuncCallLowering<SinOp>>(converter, "__ocml_sin_f32",
herhutUnsubmitted
Done
ReplyInline Actions

Can you keep these sorted?

herhut: Can you keep these sorted?
"__ocml_sin_f64");
} }
std::unique_ptr<OperationPass<gpu::GPUModuleOp>> std::unique_ptr<OperationPass<gpu::GPUModuleOp>>
mlir::createLowerGpuOpsToROCDLOpsPass(unsigned indexBitwidth) { mlir::createLowerGpuOpsToROCDLOpsPass(unsigned indexBitwidth) {
return std::make_unique<LowerGpuOpsToROCDLOpsPass>(indexBitwidth); return std::make_unique<LowerGpuOpsToROCDLOpsPass>(indexBitwidth);
} }