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.

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

Support ICC in configure #70013

Closed
zware opened this issue Dec 9, 2015 · 6 comments
Closed

Support ICC in configure #70013

zware opened this issue Dec 9, 2015 · 6 comments
Labels
build The build process and cross-build type-feature A feature request or enhancement

Comments

@zware
Copy link
Member

zware commented Dec 9, 2015

BPO 25827
Nosy @brettcannon, @doko42, @bitdancer, @skrah, @zware
Files
  • configure_icc.diff
  • configure_icc.v2.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-12-22.03:06:32.810>
    created_at = <Date 2015-12-09.07:45:54.623>
    labels = ['type-feature', 'build']
    title = 'Support ICC in configure'
    updated_at = <Date 2015-12-22.03:06:32.809>
    user = 'https://github.com/zware'

    bugs.python.org fields:

    activity = <Date 2015-12-22.03:06:32.809>
    actor = 'zach.ware'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-12-22.03:06:32.810>
    closer = 'zach.ware'
    components = ['Build']
    creation = <Date 2015-12-09.07:45:54.623>
    creator = 'zach.ware'
    dependencies = []
    files = ['41272', '41359']
    hgrepos = []
    issue_num = 25827
    keywords = ['patch']
    message_count = 6.0
    messages = ['256141', '256183', '256282', '256708', '256807', '256822']
    nosy_count = 7.0
    nosy_names = ['brett.cannon', 'doko', 'r.david.murray', 'skrah', 'python-dev', 'zach.ware', 'alecsandru.patrascu']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25827'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @zware
    Copy link
    Member Author

    zware commented Dec 9, 2015

    Attached is a patch that adds support for ICC to configure, making it easier to build with ICC on Unix and adjusting arguments to better fit ICC.

    • Adds '--with-icc' argument to configure, which sets 'CC=icc' and 'CXX=icpc'
    • Adds support for ICC PGO
    • Prevents '-Wno-unused-result' from being added to CFLAGS if CC=icc (bpo-24709)
    • Adds '-fp-model strict' to BASECFLAGS (I'm not 100% sure that's the right place, perhaps CFLAGS_NODIST would be better?). Adding '-fp-model strict' clears up all the failures on the Ubuntu ICC Non-Debug buildbot, and most of the failures I get on OSX with -O3. From a quick run of the 'math' benchmarks (perf.py -b math), adding '-fp-model strict' does not hurt performance (with the flag was always faster or negligibly slower).

    Be sure to run autoreconf before testing.

    @zware zware added build The build process and cross-build type-feature A feature request or enhancement labels Dec 9, 2015
    @bitdancer
    Copy link
    Member

    This patch looks correct to me, but I don't have much experience with configure. Are the .dyn files icc's profiling output?

    I'm adding Alecsandru and Brett as nosy since they worked on the PGO stuff. I'm also adding Stefan since he's shown interest in icc.

    @alecsandrupatrascu
    Copy link
    Mannequin

    alecsandrupatrascu mannequin commented Dec 12, 2015

    Thank you David for including me in this issue.

    On ICC, when executing the instrumented applications, it will generate dynamic information file that has a unique name and .dyn suffix. From my previous experience with GCC and CLANG PGO patch (bpo-24915), also the output of the autoconf will have to be included in the final diff.

    Regarding the place for adding the '-fp-model strict' flags, I had the same dillema when working on the LTO patch (bpo-25702), because I needed the flags to be propagated to _all_ compilation and linking phases, and finally decided to add them to the CONFIGURE_CFLAGS, CONFIGURE_CFLAGS_NODIST, CONFIGURE_CPPFLAGS, CONFIGURE_LDFLAGS variables in the Makefile.pre.in file. You can try there also and keep the BASECFLAGS clean .

    @zware
    Copy link
    Member Author

    zware commented Dec 18, 2015

    After testing, I think I like '-fp-model strict' better in CFLAGS_NODIST. On Ubuntu, CC=gcc ./python -m test.test_distutils passes when the flag is in CFLAGS_NODIST, but fails when it's in BASECFLAGS. On OSX, it fails both ways, due to deployment target issues; I suspect that's probably solvable by setting some other environment variable correctly (I don't know enough about OSX to say for sure, though). I don't think this patch needs to go as far as the LTO patch does to add the argument to CFLAGS; most third party extensions probably won't necessitate '-fp-model strict', those that do can add it themselves.

    On the other hand, 2.7 doesn't have CFLAGS_NODIST (and I don't want to add it), so BASECFLAGS is about the only option there.

    Barring objections, I'll commit this new patch in a few days.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 21, 2015

    New changeset d7b5c2f99a99 by Zachary Ware in branch '2.7':
    Issue bpo-25827: Add support for ICC to configure
    https://hg.python.org/cpython/rev/d7b5c2f99a99

    New changeset c5e419464585 by Zachary Ware in branch '3.5':
    Issue bpo-25827: Add support for ICC to configure
    https://hg.python.org/cpython/rev/c5e419464585

    New changeset 29ea3827cfaa by Zachary Ware in branch 'default':
    Issue bpo-25827: Merge with 3.5
    https://hg.python.org/cpython/rev/29ea3827cfaa

    @zware
    Copy link
    Member Author

    zware commented Dec 22, 2015

    The buildbots appear to be happy with this, so I'm closing the issue. The intel-ubuntu-icc Non-Debug builders each had their first green build after this changeset. The OSX ICC builder still has some issues, but they seem to be OSX specific (see bpo-25589).

    @zware zware closed this as completed Dec 22, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    build The build process and cross-build type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants