-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[llvm:codegen] Add lowering to all intrinsics available in math.h.
#162825
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
base: main
Are you sure you want to change the base?
Conversation
This PR adds support to `IntrinsicLowering` for all intrinsics that correspond to functions in `math.h`. Such lowerings existed previously for a subset of them but others (such as `tanh`) were missing. This lead to those intrinsics not working in the LLVM interpreter. The PR is split in three commits to make its changes more transparent: The first commit replaces the repeated logic in the various `case` statements of the corresponding intrinsics with a newly introduced macro while keeping the order of the supported intrinsics. The second commit reorders the existing calls to the macro. The third commit introduces new macro calls for previously unsupported intrinsics. By looking at the commits in isolation, it is more easy to see that no unintended changes have happened. I have produced the list of to-be-supported intrinsics by extracting the functions in `math.h` with a bash one-liner and joined that with the list of LLVM intrinsics produced with another bash one-liner, both of which I will share in the comments of the PR.
|
I produced the list of functions in grep -E "^__MATHCALL(_VEC|X| )" /usr/include/bits/mathcalls.h \
| sed -r "s-__MATHCALL(_VEC|X| ).?\(--; s|,.*||" \
| sort > math.lstI produced the list of intrinsics with the following command: tail -n+11 build/include/llvm/IR/IntrinsicEnums.inc \
| head -n-13 | cut -f1 -d, | cut -f1 -d= | tr -d " " \
| sort > intrinsics.lstNow, the intersection, which are the LLVM intrinsics that have a function with the same name in join math.lst intrinsics.lst | sort > joined.lstFinally, one can print the list of macro calls with: cat joined.lst | while read line; do echo "MATH_INTRINSIC_CASE($line)"; done |
math.h.math.h.
arsenm
left a comment
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.
This needs comprehensive test coverage
|
I haven't work with codegen before and only limited on LLVM outside of MLIR. Can you give me some pointers? |
arsenm
left a comment
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'm not really sure why we have this utility in the first place. I thought this was used by FastISel, but that appears to not be the case (maybe it used to be?).
Why would the interpreter use it? Surely the interpreter should just be directly calling the appropriate functions to implement the intrinsics? I'm not sure why it would do this extra step of converting to the libcall. This is also broken because it ignores vectors and most FP types.
This also isn't considering if RuntimeLibcallsInfo reports the function as available. If you wanted to test this in the most straightforward way, there should be a standalone utility pass.
I'm thinking we shouldn't have IntrinsicLowering at all. SPIRV uses this for bswap, but it really shouldn't. It should just use the normal legalizer
|
Quick update from my side: We stopped using the LLVM interpreter in the meantime and use ORC jitting instead, so I won't be working on this for the time being. FWIW, it sounds like the next steps include figuring out whether or not the interpreter should use this at all. |
|
Agree with @arsenm that the interpreter shouldn't be using intrinsic lowering. That's a very roundabout way to go about things. |
|
Converted to draft because it seems like this isn't the agreed-upon next step and nobody is actively working on it. |
This PR adds support to
IntrinsicLoweringfor all intrinsics that correspond to functions inmath.h. Such lowerings existed previously for a subset of them but others (such astanh) were missing. This lead to those intrinsics not working in the LLVM interpreter.The PR is split in three commits to make its changes more transparent: The first commit replaces the repeated logic in the various
casestatements of the corresponding intrinsics with a newly introduced macro while keeping the order of the supported intrinsics. The second commit reorders the existing calls to the macro. The third commit introduces new macro calls for previously unsupported intrinsics. By looking at the commits in isolation, it is more easy to see that no unintended changes have happened.I have produced the list of to-be-supported intrinsics by extracting the functions in
math.hwith a bash one-liner and joined that with the list of LLVM intrinsics produced with another bash one-liner, both of which I will share in the comments of the PR.