-
Notifications
You must be signed in to change notification settings - Fork 56
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.
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
Add new pass for math to rocdl. #93
Conversation
llvm#93290) …280)" This reverts commit 6977bfb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR LGTM :-) feel free to wait on a secondary reviewer approval or not, I'll leave it up to you!
@@ -3559,6 +3560,14 @@ class FIRToLLVMLowering | |||
// as passes here. | |||
mlir::OpPassManager mathConvertionPM("builtin.module"); | |||
|
|||
bool isAMDGCN = fir::getTargetTriple(mod).isAMDGCN(); | |||
// If compiling for AMD target some math operations must be lowered to ocml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would it be possible to get the full name for ocml in the comments? Maybe a weird request, just unfamiliar with the library so would be handy to have in the comment! if not that's not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ocml stands for OpenCL Math Library, so I'm not sure that will help or confuse more. I should perhaps use "AMD GPU math library" or something like that.
@@ -3567,6 +3576,7 @@ class FIRToLLVMLowering | |||
mathConvertionPM.addPass( | |||
mlir::createConvertMathToFuncs(mathToFuncsOptions)); | |||
mathConvertionPM.addPass(mlir::createConvertComplexToStandardPass()); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Unnecessary space addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Jan, LGTM as well. It'd be good to add a Lit test for this as well, to make sure it doesn't inadvertently break later on.
// library calls, the rest can be converted to LLVM intrinsics, which is | ||
// handled in the mathToLLVM conversion. The lowering to libm calls is not | ||
// needed since all math operations are handled this way. | ||
if (isAMDGCN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Braces
There are math operations that are not yet being converted, but it works for most operations. The changes to the flang codegen enables the pass for AMDGCN and disables the MathToLLVM and MathToLibm passes.