Skip to content

gh-101525: Use only safe identical code folding with BOLT #134642

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gh-101525: Use only safe identical code folding with BOLT #134642

Show file tree
Hide file tree
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
50 changes: 49 additions & 1 deletion configure
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 24 additions & 1 deletion configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2129,6 +2129,29 @@ if test "$Py_BOLT" = 'true' ; then
else
AC_MSG_ERROR([merge-fdata is required for a --enable-bolt build but could not be found.])
fi

py_bolt_icf_flag="-icf=safe"
AC_CACHE_CHECK(
[whether ${LLVM_BOLT} supports safe identical code folding],
[py_cv_bolt_icf_safe],
[
saved_cflags="$CFLAGS"
saved_ldflags="$LDFLAGS"
CFLAGS="$CFLAGS_NODIST"
LDFLAGS="$LDFLAGS_NODIST"
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([[]], [[]])],
[py_cv_bolt_icf_safe=no
${LLVM_BOLT} -icf=safe -o conftest.bolt conftest$EXEEXT >&AS_MESSAGE_LOG_FD 2>&1 dnl
&& py_cv_bolt_icf_safe=yes],
[AC_MSG_FAILURE([could not compile empty test program])])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[AC_MSG_FAILURE([could not compile empty test program])])
[AC_MSG_FAILURE([could not link empty test program with -icf=safe])])

It'd be good to be a bit more descriptive than "compilation failed".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The autoconf syntax here is a little confusing. This message isn't from the attempt to optimize with -icf=safe, it's from actually compiling the test program. BOLT is a post-link optimizer, so we give it a compiled and linked executable as an argument. (There is also ICF support in many linkers, to make things more confusing.) I'm using autoconf's function for running test-compiles for the side effect of creating a binary to pass to BOLT, but we need to handle the case where the compile, for whatever reason, fails. I don't expect anyone to hit this error because if you couldn't compile programs you probably couldn't get this far in ./configure anyway, but it's good to have something for the "can't happen" case.

The pseudo-Python equivalent of this check is

py_bolt_icf_flag = "-icf=safe"

@autoconf.cache_check(
    "py_cv_bolt_icf_safe",
    f"whether {LLVM_BOLT} supports safe identical code folding",
)
def check_bolt_icf_safe():
    saved_cflags, saved_ldflags = CFLAGS, LDFLAGS
    CFLAGS, LDFLAGS = CFLAGS_NODIST, LDFLAGS_NODIST
    if autoconf.test_link(autoconf.make_program("", "")):
        py_cv_bolt_icf_safe = False
        p = subprocess.run(
            [LLVM_BOLT, "-icf=safe" "-o", "conftest.bolt", f"conftest{EXEEXT}"],
            stdout=autoconf.message_log, stderr=autoconf.message_log,
        )
        if p.returncode == 0:
            py_cv_bolt_icf_safe = True
    else:
        raise RuntimeError("could not compile empty test program")
    CFLAGS, LDFLAGS = saved_cflags, saved_ldflags
    return py_cv_bolt_icf_safe

if not check_bolt_icf_safe():
    py_bolt_icf_flag = ""

It does occur to me, writing this out, that you could theoretically hit this failure case if your compiler and linker don't support the flags needed to produce a BOLT-able binary (Wl,--emit-relocs -fno-pie -no-pie), because this is the first time we're asking autoconf to use those flags in a test-compile. I have no idea why you would ask for BOLT if you don't have a vaguely compatible compiler, though, and you'd previously get a random compile error at some later point in the process, so I don't think this is making anything worse... but we could change the message to "could not compile and link test program with flags needed for BOLT" or something.

I would also accept the argument that I need wider indentation or some comments or something to make the autoconf readable.

CFLAGS="$saved_cflags"
LDFLAGS="$saved_ldflags"
Comment on lines +2138 to +2149
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using our WITH_SAVE_ENV macro; it automatically saves and restores CFLAGS, CPPFLAGS, LDFLAGS, and LIBS:

Suggested change
saved_cflags="$CFLAGS"
saved_ldflags="$LDFLAGS"
CFLAGS="$CFLAGS_NODIST"
LDFLAGS="$LDFLAGS_NODIST"
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([[]], [[]])],
[py_cv_bolt_icf_safe=no
${LLVM_BOLT} -icf=safe -o conftest.bolt conftest$EXEEXT >&AS_MESSAGE_LOG_FD 2>&1 dnl
&& py_cv_bolt_icf_safe=yes],
[AC_MSG_FAILURE([could not compile empty test program])])
CFLAGS="$saved_cflags"
LDFLAGS="$saved_ldflags"
WITH_SAVE_ENV([
CFLAGS="$CFLAGS_NODIST"
LDFLAGS="$LDFLAGS_NODIST"
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([], [])],
[py_cv_bolt_icf_safe=no
${LLVM_BOLT} -icf=safe -o conftest.bolt conftest$EXEEXT >&AS_MESSAGE_LOG_FD 2>&1 dnl
&& py_cv_bolt_icf_safe=yes],
[AC_MSG_FAILURE([could not compile empty test program])])
])

Moreover, the double brackets ([[]]) are not needed; a single pair ([]) is sufficient.

]
)
if test "$py_cv_bolt_icf_safe" = no; then
py_bolt_icf_flag=""
fi
fi

dnl Enable BOLT of libpython if built.
Expand Down Expand Up @@ -2184,7 +2207,7 @@ then
-reorder-blocks=ext-tsp
-reorder-functions=cdsort
-split-functions
-icf=1
${py_bolt_icf_flag}
-inline-all
-split-eh
-reorder-functions-use-hot-size
Expand Down
Loading