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

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

Merged

Conversation

MatthiasJReisinger
Copy link
Contributor

@MatthiasJReisinger 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
Copy link
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 A NEWS entry is required for this change label Aug 20, 2016
@MatthiasJReisinger MatthiasJReisinger force-pushed the mjr/polly/command_line_switch branch from dbe9419 to 0495dfd Compare August 20, 2016 20:08
@MatthiasJReisinger
Copy link
Contributor Author

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

Best,
Matthias

@MatthiasJReisinger MatthiasJReisinger force-pushed the mjr/polly/command_line_switch branch from 0495dfd to f2348c4 Compare August 20, 2016 20:18
@MatthiasJReisinger MatthiasJReisinger changed the title WIP: Add command line option --disable-polly 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"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should put #ifdef USE_POLLY around this

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
Copy link
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 mjr/polly/command_line_switch branch from f2348c4 to 2116a6c Compare August 22, 2016 15:48
@MatthiasJReisinger
Copy link
Contributor Author

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

@MatthiasJReisinger MatthiasJReisinger changed the title WIP: Add command line option --polly={yes|no} Add command line option --polly={yes|no} Aug 22, 2016
@vtjnash vtjnash merged commit f5018b7 into JuliaLang:master Aug 22, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 22, 2016

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

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Aug 23, 2016
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants