Skip to content

[Release] Install compiler-rt builtins during Phase 1 on AIX #81485

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

Merged
merged 2 commits into from
Feb 16, 2024
Merged

[Release] Install compiler-rt builtins during Phase 1 on AIX #81485

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
660ba3f
Update install target
azhan92 Feb 12, 2024
29d90a2
Condition only applies on AIX
alisonzhang4 Feb 14, 2024
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: 1 addition & 1 deletion llvm/utils/release/test-release.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ function build_llvmCore() {
# compiler-rt builtins is needed on AIX to have a functional Phase 1 clang.
if [ "$System" = "AIX" -o "$Phase" != "1" ]; then
BuildTarget="$BuildTarget runtimes"
InstallTarget="$InstallTarget install-runtimes"
InstallTarget="$InstallTarget install-runtimes install-builtins"
Copy link
Contributor

@amy-kwan amy-kwan Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tstellar Just wanted to check, but in the original commit that added this section, there's no explicit install-builtins so did we just miss adding it or is its absence intentional?

We realized that install-runtimes does not actually install the compiler-rt builtins, so it appears that install-builtins is needed here (at least on AIX).

Also, it would seem that this section can run on non-AIX systems, and on Phase 2, and I'm not quite sure if that matches the comment on 534. Is this also intentional?

Copy link
Collaborator

@tstellar tstellar Feb 12, 2024

Choose a reason for hiding this comment

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

@tstellar Just wanted to check, but in the original commit that added this section, there's no explicit install-builtins so did we just miss adding it or is its absence intentional?

I just missed adding install-builtins I didn't know it was required. Does it work if you do only install-builtins and not install-runtimes.

Also, it would seem that this section can run on non-AIX systems, and on Phase 2, and I'm not quite sure if that matches the comment on 534. Is this also intentional?

This was another mistake. You are correct there is a bug in the current logic.

Copy link
Contributor Author

@azhan92 azhan92 Feb 13, 2024

Choose a reason for hiding this comment

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

It does work with just install-builtins and not install-runtimes

Copy link
Collaborator

@hubert-reinterpretcast hubert-reinterpretcast Feb 13, 2024

Choose a reason for hiding this comment

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

This was another mistake. You are correct there is a bug in the current logic.

@tstellar:
Was the intent for only AIX to have the extra targets added on Phase 1 and 2?
(I would be surprised if it was needed for only Phase 1 and not Phase 2).

fi
fi

Expand Down