Skip to content

Remove DXC_BUILD_ARCH variable#4156

Merged
llvm-beanz merged 4 commits intomicrosoft:mainfrom
llvm-beanz:cbieneman/cleanup-windows-targets
Nov 22, 2022
Merged

Remove DXC_BUILD_ARCH variable#4156
llvm-beanz merged 4 commits intomicrosoft:mainfrom
llvm-beanz:cbieneman/cleanup-windows-targets

Conversation

@llvm-beanz
Copy link
Copy Markdown
Collaborator

This commit simplifies the cross-targeting build support to not require
specifying the build architecture explicitly. Instead it is derived from
the build tools used in the generation.

I've tested this change locally for Win32, x64 and arm64.

@llvm-beanz llvm-beanz requested review from hekota and pow2clk December 17, 2021 02:56
@AppVeyorBot
Copy link
Copy Markdown

Comment thread cmake/modules/FindTAEF.cmake Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please check if hctbuild -arm64ec works? I suspect it might not in this case. There is no arm64ec or arm64x directory in WinSDK, we should use the arm64 one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm struggling to get arm64ec to build locally without this change. I'm sure I don't have my system setup correctly :(

@llvm-beanz llvm-beanz force-pushed the cbieneman/cleanup-windows-targets branch from 122c472 to b172aae Compare December 17, 2021 16:36
@AppVeyorBot
Copy link
Copy Markdown

@gongminmin
Copy link
Copy Markdown
Contributor

gongminmin commented May 27, 2022

CMAKE_<LANG>_COMPILER_ARCHITECTURE_ID is introduced in cmake 3.10. The cmake_minimum_required(VERSION 2.8.12.2) in root cmakelists may need to be changed, too.

This commit simplifies the cross-targeting build support to not require
specifying the build architecture explicitly. Instead it is derived from
the build tools used in the generation.

I've tested this change locally for Win32, x64 and arm64.
CMake 3.10 is very old, but has a feature this PR uses. Raising the
requirement should have no impact on our users.
@llvm-beanz llvm-beanz force-pushed the cbieneman/cleanup-windows-targets branch from b172aae to a6a255c Compare November 16, 2022 22:31
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Looks good. Can you confirm that arm is working as Helena was worried about?

@llvm-beanz
Copy link
Copy Markdown
Collaborator Author

Arm64 works fine for me locally, but I can't get Arm64EC to build without this change, and with it I get the same failure.

@AppVeyorBot
Copy link
Copy Markdown

@hekota
Copy link
Copy Markdown
Member

hekota commented Nov 21, 2022

Go ahead and complete it, the ARM64EC should work down the line.

@llvm-beanz
Copy link
Copy Markdown
Collaborator Author

This works on arm64ec with #4808, which works around underlying bugs in CMake and COFF object file size limitations for arm64ec.

@llvm-beanz llvm-beanz merged commit fb6b287 into microsoft:main Nov 22, 2022
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