Skip to content

[Downstream change][LV] Workaround for sincos vectorization on aarch64-amazon-linux triple #279

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

Closed
wants to merge 1 commit into from

Conversation

MacDue
Copy link
Contributor

@MacDue MacDue commented Apr 14, 2025

When the aarch64-amazon-linux triple is specified LLVM currently does not think it has a GNU environment, so looking up the "sincos" library function names via RTLIB::SINCOS_F32/64 fails, which prevents vectorizing sincos with vector libraries. The function names are needed only to find the corresponding vector mapping, so it does not matter if the scalar function exists. Given this, we can work around this issue by hardcoding names for SINCOS_F32/64 when we know we're using the name to find a vector mapping.

Downstream change: #87

When the `aarch64-amazon-linux` triple is specified LLVM currently does
not think it has a GNU environment, so looking up the "sincos" library
function names via RTLIB::SINCOS_F32/64 fails, which prevents
vectorizing sincos with vector libraries. The function names are needed
only to find the corresponding vector mapping, so it does not matter if
the scalar function exists. Given this, we can work around this issue by
hardcoding names for SINCOS_F32/64 when we know we're using the name to
find a vector mapping.
Copy link
Contributor

@pawosm-arm pawosm-arm left a comment

Choose a reason for hiding this comment

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

LG

@kiranchandramohan
Copy link
Contributor

@paulwalker-arm @mgabka Is this patch OK? We decided to go with a downstream fix since this is a regression between ACfL and ATfL.

@paulwalker-arm
Copy link
Contributor

paulwalker-arm commented Apr 15, 2025

@paulwalker-arm @mgabka Is this patch OK? We decided to go with a downstream fix since this is a regression between ACfL and ATfL.

Not really. To me this patch is simply perverting an existing interface to achieve the desired result. It would be better to figure out why this interface (or its dependent) is not already able to lookup the required system library function. Also, for systems where this function is genuinely not available we'll erroneously return a match.

My worry is that we're simply papering over missing integration required to support Amazon linux.

@MacDue
Copy link
Contributor Author

MacDue commented Apr 15, 2025

Not really. To me this patch is simply perverting an existing interface to achieve the desired result.

The basic issue is that the Amazon Linux triple is not hooked up in RuntimeLibcalls.cpp (and probably TargetParser). If Amazon Linux has a GNU environment (which I believe it does), that should be recognized by Triple::isGNUEnvironment(), and then the names for sincos will be available from getLibcallName().

I avoided fixing that for this workaround as it's not necessary if the name is only needed to find a vector mapping. Before adding the intrinsic for sincos the mappings would be found even though getLibcallName() was wrong on Amazon Linux, as the name for the scalar function could be found by just inspecting the call.

This is where the names are registered (which is not working for Amazon Linux):
https://github.com/llvm/llvm-project/blob/e1382b3b459d39659694ee854073bbdb1aa1d98d/llvm/lib/IR/RuntimeLibcalls.cpp#L176-L188

Note: Using a triple like aarch64-amazon-gnu-linux would also resolve this for Amazon Linux (without needing this workaround).

@dcandler
Copy link
Contributor

Can this not be fixed upstream instead? The other changes in #87 were in the process of upstream review, and the changes were only made to our release branch because they missed the upstream branch date.

Downstream changes should be a last resort, so there should be an attempt to upstream the work, or justify why it can't be done.

@MacDue
Copy link
Contributor Author

MacDue commented Apr 16, 2025

This PR probably is not acceptable upstream. I had another look at this and I think a "correct" fix is fairly simple. I've posted that in #286, and I believe that patch may be acceptable upstream too.

@MacDue
Copy link
Contributor Author

MacDue commented Apr 16, 2025

Note: This this is cherry-picking into a release branch (which won't happen upstream) so a downstream change is needed if a fix is desired for LLVM 20.

@MacDue
Copy link
Contributor Author

MacDue commented Apr 16, 2025

Closing in favour of #286 (as I believe that's less of a hack).

@MacDue MacDue closed this Apr 16, 2025
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.

5 participants