Page MenuHomePhabricator

[mlir] Add coro intrinsics operations to LLVM dialect
ClosedPublic

Authored by ezhulenev on Jan 21 2021, 8:56 AM.

Details

Summary

This PR only has coro intrinsics needed for the Async to LLVM lowering. Will add other intrinsics as needed in the followup PRs.

Diff Detail

Event Timeline

ezhulenev created this revision.Jan 21 2021, 8:56 AM
ezhulenev requested review of this revision.Jan 21 2021, 8:56 AM
ezhulenev edited the summary of this revision. (Show Details)Jan 21 2021, 8:57 AM
ezhulenev updated this revision to Diff 318230.Jan 21 2021, 8:58 AM

remove dump input

ftynse requested changes to this revision.Jan 22 2021, 4:27 AM

Have you considered putting these in a separate dialect that just uses LLVMOpBase.td, similarly to what we have for NVVM or AVX512? My general take on it is that intrinsics listed in https://llvm.org/docs/LangRef.html#intrinsic-functions can stay in the "main" dialect, but "target-specific" intrinsics are better off in separate dialects. I understand there are pros and cons of doing this, so I won't block this commit only because of it.

mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
98–109

I'd rather do something like def LLVM_I8PointerType : LLVM_PointerTo<LLVM_i8>; and use it everywhere. Otherwise, this looks like there is an "opaque pointer" type different from "pointer to i8", which is not the case.

This revision now requires changes to proceed.Jan 22 2021, 4:27 AM
ezhulenev updated this revision to Diff 318545.Jan 22 2021, 9:11 AM

Add LLVM_i?Ptr type constraint

ezhulenev marked an inline comment as done.Jan 22 2021, 9:13 AM

Have you considered putting these in a separate dialect that just uses LLVMOpBase.td, similarly to what we have for NVVM or AVX512? My general take on it is that intrinsics listed in https://llvm.org/docs/LangRef.html#intrinsic-functions can stay in the "main" dialect, but "target-specific" intrinsics are better off in separate dialects. I understand there are pros and cons of doing this, so I won't block this commit only because of it.

Coro intrinsics are actually defined in the "main" LLVM intrinsic definition file, so I think it makes sense to keep them on LLVMOpBase:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/Intrinsics.td#L1184

mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
98–109

Added a LLVM_i8Ptr constraint similar to simple LLVM_i8

ezhulenev marked an inline comment as done.Jan 22 2021, 9:14 AM
mehdi_amini accepted this revision.Jan 22 2021, 9:41 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 22 2021, 10:02 AM
This revision was automatically updated to reflect the committed changes.