Skip to content

[MLIR][NVVM] Enable nvvm intrinsics import #68843

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

Jump to bottom
Merged
merged 12 commits into from
Dec 12, 2023
Merged

[MLIR][NVVM] Enable nvvm intrinsics import #68843

merged 12 commits into from
Dec 12, 2023

Conversation

ivanradanov
Copy link
Contributor

No description provided.

@ivanradanov ivanradanov changed the title [MLIR][NVVM] Enable LLVMIR module import with nvvm intrinsics [MLIR][NVVM] Enable nvvm intrinsics import Oct 12, 2023
@ivanradanov ivanradanov force-pushed the nvvmimport branch 2 times, most recently from c9a1700 to 547937b Compare October 12, 2023 13:11
@ivanradanov
Copy link
Contributor Author

@ftynse

@ftynse ftynse requested a review from gysit October 17, 2023 23:42
@ftynse
Copy link
Member

ftynse commented Oct 17, 2023

@gysit is more knowledgeable about this part of the code so I'll defer to him. One general note is that I would split out NVVM into separate files, the same as we do on the MLIR->LLVMIR conversion side, rather than shove everything together.

@gysit
Copy link
Contributor

gysit commented Oct 18, 2023

I would second @ftynse here that we should separate the import of NVVM intrinsics. The import is very similar to the export in the sense that you can implement an interface for your target that implements the actual conversion. There are some difficulties to solve there since the intrinsics don't have a dialect associated in LLVM IR. The interface therefore needs to communicate which intrinsics it supports (https://discourse.llvm.org/t/rfc-extensible-llvm-ir-import/67256).

In principle, you want to build a counterpart to LLVMDialectLLVMIRImportInterface inside the mlir/lib/Target/LLVMIR/Dialect/NVVM folder. It needs to overwrite the relevant methods (I guess you do not care about metadata import). The getSupportedIntrinsics method returns the supported intrinsics and convertIntrinsicImpl. Most of the code in this methods can be autogenerated using tablegen. Finally you need to make sure to register the new translation interface.

I think you are the first one that tries this mechanism for intrinsics (at least upstream). However, things should work analogous to the LLVMIR import.

@ivanradanov
Copy link
Contributor Author

ivanradanov commented Nov 7, 2023 •
edited
Loading

@gysit Thank you for the explanation! How does it look now?

gysit
gysit reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

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

Nice! The structure looks good to me.

We probably want to increase the test coverage a little bit though.

mlir/test/Target/LLVMIR/Import/intrinsic.ll Outdated
@@ -865,6 +865,14 @@ define float @ssa_copy(float %0) {
ret float %2
}

; CHECK-LABEL: llvm.func @nvvm
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the tests into an nvvmir.ll file in the same folder to follow the same structure as in the export?

Can you also add a test for every intrinsic? I know it seems like a lot of copy paste but it still seems helpful to exercise the import of all of them in case things get refactored in LLVM land for example.

@gysit
Copy link
Contributor

gysit commented Nov 8, 2023

Let me add @Dinistro as another reviewer since I will be leaving for a vacation tonight.

@gysit gysit requested a review from Dinistro November 8, 2023 10:21
@ivanradanov
Copy link
Contributor Author

@gysit @Dinistro

I added a test which includes all of the currently supported nvvm intrinsics for import.

To be automatically imported the nvvm ops must inherit from NVVM_IntrOp, and most of them currently inherit from NVVM_Op. I will be looking into whether the remaining can be made to inherit from IntrOp but I think that is best left for a separate PR?

gysit
gysit approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

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

LGMT modulo nit.

Yes I think it makes sense to separate the support for the remaining intrinsics into a followup PR.

Dinistro
Dinistro approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@Dinistro Dinistro left a comment

Choose a reason for hiding this comment

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

LGTM % minor nits.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp Outdated
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//

#include "mlir/Dialect/DLTI/DLTI.h"
#include "mlir/Dialect/LLVMIR/NVVMDialect.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This include doesn't seem necessary.

@gysit
Copy link
Contributor

gysit commented Dec 11, 2023

@ivanradanov do you need help with landing the pr?

Also if you do not have commit access yet you may want ask for it soonish https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

@ivanradanov
Copy link
Contributor Author

ivanradanov commented Dec 11, 2023 •
edited
Loading

@ivanradanov do you need help with landing the pr?

Also if you do not have commit access yet you may want ask for it soonish https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

If you could do it for me that would be great.

(Thank you for the link, I asked for commit access but do not have it yet)

Edit: got it!

@ivanradanov ivanradanov merged commit d5fb4c0 into llvm:main Dec 12, 2023
@ivanradanov ivanradanov deleted the nvvmimport branch December 12, 2023 04:35
@gysit
Copy link
Contributor

gysit commented Dec 12, 2023

Ah I see, the commit right process was faster then me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gysit gysit gysit approved these changes

@Dinistro Dinistro Dinistro approved these changes

Assignees
No one assigned
Labels
mlir:llvm mlir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ivanradanov @ftynse @gysit @Dinistro @llvmbot