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.

Sign up for GitHub

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

Jump to bottom

add support for riscv64 #752

Merged
merged 1 commit into from
Jun 26, 2024
Merged

add support for riscv64 #752

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmake/modules/SwiftSupport.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ function(get_swift_host_arch result_var_name)
set("${result_var_name}" "i686" PARENT_SCOPE)
elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "wasm32")
set("${result_var_name}" "wasm32" PARENT_SCOPE)
elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "riscv64")
Copy link

Choose a reason for hiding this comment

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

It looks like this is just for computing the module triple.
If that's the case, rather than continuing to extend this tower of if statements, would you be okay swapping out the module triple computation for this:

if(NOT SwiftFormat_MODULE_TRIPLE)
  set(module_triple_command "${CMAKE_Swift_COMPILER}" -print-target-info)
  if(CMAKE_Swift_COMPILER_TARGET)
    list(APPEND module_triple_command -target ${CMAKE_Swift_COMPILER_TARGET})
  endif()
  execute_process(COMMAND ${module_triple_command} OUTPUT_VARIABLE target_info_json)
  string(JSON SwiftFormat_MODULE_TRIPLE GET "${target_info_json}" "target" "moduleTriple")
endif()

It scales better and means that this will build for anything that the compiler supports, rather than what we've manually listed out.

Please see the conversation on swiftlang/swift-corelibs-foundation#4959

https://github.com/apple/swift-corelibs-foundation/pull/4959/files#diff-9179d83b9cabd38d5049f3095d2c31d286f406b53443027b1bac119ecc4408c1R38-R54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etcwilde, I understand your reasoning for suggesting this change and if this was the only project with this change I would agree.
Unfortunately it is not. This is just one of 19 separate projects in the swift toolchain that require these changes.
https://github.com/orgs/swift-riscv/projects/2
Some of these have already been approved and merged.
For the sake of code conformity across all the different projects I would rather leave this PR as I submitted it if that is acceptable.
Thanks.

Copy link

Choose a reason for hiding this comment

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

For the ones that have merged, sure, we can update them later when the next architecture comes along.
I think we've hit a critical mass where expecting identical behavior across all of the repos is unlikely to happen and I would prefer to see incremental improvements over time over not improving anything to keep uniformity.
Sort of like how LLVM rolls out changes to policies, instead of making a gigantic patch that updates all of the code, folks are supposed to clean up the code around what they're working on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etcwilde so you are refusing to accept this PR as is?
If so, please state this clearly so I can take further action.

Copy link

Choose a reason for hiding this comment

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

Yes, I have requested changes and given my reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etcwilde thats unfortunate.

set("${result_var_name}" "riscv64" PARENT_SCOPE)
else()
message(FATAL_ERROR "Unrecognized architecture on host system: ${CMAKE_SYSTEM_PROCESSOR}")
endif()
Expand Down