[Release] Install compiler-rt builtins during Phase 1 on AIX #81485
Conversation
llvm/utils/release/test-release.sh
Outdated
| @@ -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" | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@tstellar Just wanted to check, but in the original commit that added this section, there's no explicit
install-builtinsso 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.
There was a problem hiding this comment.
It does work with just install-builtins and not install-runtimes
There was a problem hiding this comment.
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).
|
@hubert-reinterpretcast I was trying to copy the logic that was there before, so whatever that was is correct. |
@tstellar, it seems to me that there was special treatment of neither AIX nor Phase 2 in the state prior to fcae8ce, so (since we do know that special AIX behavior is intended at least for Phase 1) I interpret your statements as there being no broad intent for special Phase 2 behavior. @azhan92, can you see if a combination of the corrected install target with a condition to apply only for Phase 1 on AIX works? If not, then also apply for Phase 2 on AIX. |
It looks like this statement is necessary for Phase 2 on AIX as well. Should I just remove the |
That seems OK to me. |
|
/cherry-pick 3af5c98 |
) The current test-release.sh script does not install the necessary compiler-rt builtin's during Phase 1 on AIX, resulting on a non-functional Phase 1 clang. Futhermore, the installation is also necessary for Phase 2 on AIX. Co-authored-by: Alison Zhang <[email protected]> (cherry picked from commit 3af5c98)
|
/pull-request #83323 |
) The current test-release.sh script does not install the necessary compiler-rt builtin's during Phase 1 on AIX, resulting on a non-functional Phase 1 clang. Futhermore, the installation is also necessary for Phase 2 on AIX. Co-authored-by: Alison Zhang <[email protected]> (cherry picked from commit 3af5c98)
The current test-release.sh script does not install the necessary compiler-rt builtin's during Phase 1 on AIX, resulting on a non-functional Phase 1 clang.