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

Add command line option --polly={yes|no} #18159

Merged
merged 1 commit into from Aug 22, 2016

Conversation

5 participants
@MatthiasJReisinger
Contributor

MatthiasJReisinger commented Aug 20, 2016

When this option is passed @polly declarations will be ignored. This facilitates debugging or evaluating performance differences between using/not using Polly without having to manually remove @polly declarations from functions.

I also wanted to ask you for some advice on the following issues: The polly field in the JLOptions type will also be present in a non-Polly build (USE_POLLY := 0) because I don't know how I could hide it in this case. Do you think that there would be a better alternative for this or would that be acceptable?

Furthermore, I also wanted to add according test cases to test/cmdlineargs.jl:

@test readchomp(`$exename -E "Bool(Base.JLOptions().polly)"`) == "true"
@test readchomp(`$exename --disable-polly -E "Bool(Base.JLOptions().polly)"`) == "false"

But the problem for the second test case is that in a non-Polly build --disable-polly would not be available which would make this test fail. A possible solution would be to add the following to the build_h.jl.phy target in base/Makefile:

ifeq ($(USE_POLLY), 1)
    @echo "const USE_POLLY = true" >> $@
else
    @echo "const USE_POLLY = false" >> $@
endif

Then it should be possible to make the above test cases conditional by adding:

if Base.USE_POLLY
    @test readchomp(`$exename -E "Bool(Base.JLOptions().polly)"`) == "true"
    @test readchomp(`$exename --disable-polly -E "Bool(Base.JLOptions().polly)"`) == "false"
end

Do you think that this would be a convenient solution?

Thank you in advance for your help!

Best,
Matthias

@tobig @vtjnash @timholy

@tkelman

This comment has been minimized.

Contributor

tkelman commented Aug 20, 2016

probably simpler to always have the option (--polly=no would be more consistent with others) but have it not do anything in a non polly build

@tkelman tkelman added the needs news label Aug 20, 2016

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/command_line_switch branch from dbe9419 to 0495dfd Aug 20, 2016

@MatthiasJReisinger

This comment has been minimized.

Contributor

MatthiasJReisinger commented Aug 20, 2016

Thank you for the feedback @tkelman! I updated the PR accordingly.

Best,
Matthias

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/command_line_switch branch from 0495dfd to f2348c4 Aug 20, 2016

@MatthiasJReisinger MatthiasJReisinger changed the title from WIP: Add command line option --disable-polly to WIP: Add command line option --polly={yes|no} Aug 20, 2016

@@ -46,6 +46,7 @@ static const char opts[] =
" -O, --optimize={0,1,2,3} Set the optimization level (default 2 if unspecified or 3 if specified as -O)\n"
" --inline={yes|no} Control whether inlining is permitted (overrides functions declared as @inline)\n"
" --check-bounds={yes|no} Emit bounds checks always or never (ignoring declarations)\n"
" --polly={yes|no} Enable or disable the polyhedral optimizer Polly (overrides @polly declaration)\n"

This comment has been minimized.

@vtjnash

vtjnash Aug 22, 2016

Member

I think we should put #ifdef USE_POLLY around this

Add command line option --polly={yes|no}
Passing `--polly=no` will cause `@polly` declarations to be ignored.
This facilitates debugging or evaluating performance differences between
using/not using Polly without having to manually remove `@polly`
declarations from functions.
@vtjnash

This comment has been minimized.

Member

vtjnash commented Aug 22, 2016

lgtm. if we add another similar option in the future, maybe we should transition to a feature list (e.g. --features=with-polly,with-other,without-that, but I think it would be premature to build that without knowing what the other cases look like).

is this still WIP, or can we merge it?

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/command_line_switch branch from f2348c4 to 2116a6c Aug 22, 2016

@MatthiasJReisinger

This comment has been minimized.

Contributor

MatthiasJReisinger commented Aug 22, 2016

Thank you for the feedback! It's no longer WIP.

@MatthiasJReisinger MatthiasJReisinger changed the title from WIP: Add command line option --polly={yes|no} to Add command line option --polly={yes|no} Aug 22, 2016

@vtjnash vtjnash merged commit f5018b7 into JuliaLang:master Aug 22, 2016

2 of 3 checks passed

ci/circleci No test commands were found
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tkelman

This comment has been minimized.

Contributor

tkelman commented Aug 22, 2016

New command-line flags should be mentioned in NEWS.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment