Skip to content
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

RFC: Remove sqrt_llvm intrinsic #43869

Closed
wants to merge 1 commit into from
Closed

RFC: Remove sqrt_llvm intrinsic #43869

wants to merge 1 commit into from

Conversation

Keno
Copy link
Member

@Keno Keno commented Jan 19, 2022

This is more of a "Do we want to move in this direction RFC". As mentioned in #43786,
we currently have three implementations of these intrinsics:

  1. The code generated by LLVM for the intrinsic
  2. The code LLVM uses for constant folding the intrinsic
  3. Our own runtime intrinsic used by the interpreter

This basically removes the third one, which will be required if we want to do
something about #26434 because we just forward these to libm. Of course we'll
still have to do something to teach LLVM how to constant fold these in a manner
compatible with what will actually end up running, but that's a separate issue.

This used to have constprop implications but #43852 will fix that, so this is definitely viable
if we want to go there.

(Of course there's a couple other intrinsics here that are similar, so we'd want to do the same thing).

This is more of a "Do we want to move in this direction RFC". As mentioned in #43786,
we currently have three implementations of these intrinsics:

1. The code generated by LLVM for the intrinsic
2. The code LLVM uses for constant folding the intrinsic
3. Our own runtime intrinsic used by the interpreter

This basically removes the third one, which will be required if we want to do
something about #26434 because we just forward these to libm. Of course we'll
still have to do something to teach LLVM how to constant fold these in a manner
compatible with what will actually end up running, but that's a separate issue.
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I think it would be generally better to rely more on having correct intrinsics in the interpreter, and move in the opposite direction, rather than relying more heavily on the availability of codegen.

@vtjnash vtjnash closed this Jun 10, 2022
@vtjnash vtjnash deleted the kf/rmsqrtllvm branch June 10, 2022 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants