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

[compiler-rt] Fix detecting _Float16 support for secondary targets (#117813) #10366

Merged

Conversation

etcwilde
Copy link

@etcwilde etcwilde commented Mar 27, 2025

Cherry-picking from llvm#117813 and llvm#133952

It turns out we were not passing -m32 to the check_c_source_compiles() invocation since CMAKE_REQUIRE_FLAGS needs to be string separated list and we were passing a ;-separated CMake list which appears to be parsed by CMake as 'ignore all arguments beyond the first'. Fix this by transforming the list to a command line first.

With this change, Clang 17 no longer claims to support _Float16 for i386.

And to pass all of the required flags:

The try-compile mechanism requires that CMAKE_REQUIRED_FLAGS is a
space-separated string instead of a list of flags. The original code
expanded BUILTIN_FLAGS into CMAKE_REQUIRED_FLAGS as a
space-separated string and then would overwrite CMAKE_REQUIRED_FLAGS
with TARGET_${arch}_CFLAGS prepended to the unexpanded
BUILTIN_CFLAGS_${arch}. This resulted in the first two arguments being
passed into the try-compile invocation, but dropping the other arguments
listed in BUILTIN_CFLAGS_${arch}.

This patch appends TARGET_${arch}_CFLAGS and BUILTIN_CFLAGS_${arch} to
CMAKE_REQUIRED_FLAGS before expanding CMAKE_REQUIRED_FLAGS as a
space-separated string. This passes any pre-set required flags, in addition to
all of the builtin and target flags to the Float16 detection.

(cherry picked from commit a4c8ef0 and commit 0d3f5ec)

@etcwilde etcwilde requested a review from a team as a code owner March 27, 2025 21:39
@etcwilde
Copy link
Author

@swift-ci please test

@etcwilde
Copy link
Author

macOS failure:

Resolving deltas:  49% (3369949/6877446)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2846)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:2185)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:635)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$GitCommandMasterToSlaveCallable.call(RemoteGitImpl.java:170)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$GitCommandMasterToSlaveCallable.call(RemoteGitImpl.java:161)
	at hudson.remoting.UserRequest.perform(UserRequest.java:211)
	at hudson.remoting.UserRequest.perform(UserRequest.java:54)
	at hudson.remoting.Request$2.run(Request.java:377)
	at hudson.remoting.InterceptingExecutorService.lambda$wrap$0(InterceptingExecutorService.java:78)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
	Suppressed: hudson.remoting.Channel$CallSiteStackTrace: Remote call to macos-node-i-0efc27fdcab43d410
		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1787)
		at hudson.remoting.UserRequest$ExceptionResponse.retrieve(UserRequest.java:356)
		at hudson.remoting.Channel.call(Channel.java:1003)
		at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.execute(RemoteGitImpl.java:153)
		at jdk.internal.reflect.GeneratedMethodAccessor430.invoke(Unknown Source)
		at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
		at java.base/java.lang.reflect.Method.invoke(Method.java:566)
		at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.invoke(RemoteGitImpl.java:138)
		at com.sun.proxy.$Proxy87.execute(Unknown Source)
		at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:997)
		at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1241)
		at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1305)
		at org.jenkinsci.plugins.workflow.steps.scm.SCMStep.checkout(SCMStep.java:136)
		at org.jenkinsci.plugins.workflow.steps.scm.SCMStep$StepExecutionImpl.run(SCMStep.java:101)
		at org.jenkinsci.plugins.workflow.steps.scm.SCMStep$StepExecutionImpl.run(SCMStep.java:88)
		at org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution.lambda$start$0(SynchronousNonBlockingStepExecution.java:47)
		at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
		at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
		at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
		at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
		at java.base/java.lang.Thread.run(Thread.java:829)
ERROR: Error fetching remote repo 'origin'
ERROR: Maximum checkout retry attempts reached, aborting

@etcwilde
Copy link
Author

@swift-ci please test macOS

@etcwilde
Copy link
Author

etcwilde commented Apr 2, 2025

The cherry-pick doesn't pass all of the flags, only the first two. I have llvm#133952 posted to LLVM upstream that will pass all of the flags to the try-compile. I'll cherry-pick that one onto this PR once I get it landed.

arichardson and others added 2 commits April 4, 2025 09:07
…lvm#117813)

It turns out we were not passing -m32 to the check_c_source_compiles()
invocation since CMAKE_REQUIRE_FLAGS needs to be string separated list
and
we were passing a ;-separated CMake list which appears to be parsed by
CMake as 'ignore all arguments beyond the first'.
Fix this by transforming the list to a command line first.

With this change, Clang 17 no longer claims to support _Float16 for
i386.

(cherry picked from commit a4c8ef0)
)

The try-compile mechanism requires that `CMAKE_REQUIRED_FLAGS` is a
space-separated string instead of a list of flags. The original code
expanded `BUILTIN_FLAGS` into `CMAKE_REQUIRED_FLAGS` as a
space-separated string and then would overwrite `CMAKE_REQUIRED_FLAGS`
with `TARGET_${arch}_CFLAGS` prepended to the unexpanded
`BUILTIN_CFLAGS_${arch}`. This resulted in the first two arguments being
passed into the try-compile invocation, but dropping the other arguments
listed in `BUILTIN_CFLAGS_${arch}`.

This patch appends `TARGET_${arch}_CFLAGS` and `BUILTIN_CFLAGS_${arch}` to
`CMAKE_REQUIRED_FLAGS` before expanding CMAKE_REQUIRED_FLAGS as a
space-separated string. This passes any pre-set required flags, in addition to
all of the builtin and target flags to the Float16 detection.

(cherry picked from commit 0d3f5ec)
@etcwilde etcwilde force-pushed the ewilde/fix-float16-detection branch from e5eae76 to e958952 Compare April 4, 2025 16:08
@etcwilde
Copy link
Author

etcwilde commented Apr 4, 2025

@swift-ci please test

@etcwilde etcwilde merged commit 140bf2f into swiftlang:stable/20240723 Apr 7, 2025
3 checks passed
@etcwilde etcwilde deleted the ewilde/fix-float16-detection branch April 7, 2025 19:38
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.

4 participants