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

Polly plugin not auto-loaded #44346

Closed
berolinux opened this issue Feb 24, 2020 · 13 comments
Closed

Polly plugin not auto-loaded #44346

berolinux opened this issue Feb 24, 2020 · 13 comments
Labels

Comments

@berolinux
Copy link

@berolinux berolinux commented Feb 24, 2020

Bugzilla Link 45001
Resolution FIXED
Resolved on Feb 26, 2020 11:03
Version 10.0
OS Linux
Blocks #43900
CC @zmodem,@Meinersbur,@slacka,@serge-sans-paille
Fixed by commit(s) 6369b9b

Extended Description

In a recent 10.0 branch snapshot build with polly enabled, trying to use polly the way it is described in the polly man page doesn't work:

$ clang -O3 -mllvm -polly test.c
clang (LLVM option parsing): Unknown command line argument '-polly'. Try: 'clang (LLVM option parsing) --help'
clang (LLVM option parsing): Did you mean '--color'?

The problem is that the plugin isn't loaded automatically -- loading the plugin manually "fixes" it.

$ clang -Xclang -load -Xclang LLVMPolly.so -O3 -mllvm -polly ~/test.c
[works]

Either autoload should be restored, or the polly man page should be updated to mention the plugin needs to be loaded.

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 24, 2020

Fix under review: https://reviews.llvm.org/D72372

Unfortunately, for clang-10, we probably will not cherry-pick it, for the same reasons as for #120.

I would update the polly docs, but the website is currently not updating.

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 24, 2020

Committed as 6369b9b

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 24, 2020

Reopening because the issue is not solved yet for clang-10.

@berolinux
Copy link
Author

@berolinux berolinux commented Feb 24, 2020

I can confirm that the fix works on top of the 10 branch.

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 24, 2020

Thanks for confirming!

@zmodem
Copy link
Collaborator

@zmodem zmodem commented Feb 25, 2020

Fix under review: https://reviews.llvm.org/D72372

Unfortunately, for clang-10, we probably will not cherry-pick it, for the
same reasons as for #120.

I think the fact that more fixes (D72372) are needed confirms my decision in #120 that this is not stable enough to be merged.

I would update the polly docs, but the website is currently not updating.

Sounds like everything is broken :(

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 25, 2020

I think the fact that more fixes (D72372) are needed confirms my decision in
#120 that this is not stable
enough to be merged.

The commit 6369b9b only changes a CMake default value to what it was in the 9.0 branch. I'd consider it relatively risk-free.

An alternative would be to revert
https://reviews.llvm.org/rG24ab9b537e61b3fe5e6a1019492ff6530d82a3ee
in the clang-10 branch which introduced this change of default value.

@zmodem
Copy link
Collaborator

@zmodem zmodem commented Feb 26, 2020

An alternative would be to revert
https://reviews.llvm.org/rG24ab9b537e61b3fe5e6a1019492ff6530d82a3ee
in the clang-10 branch which introduced this change of default value.

That would be my preferred solution, except that for something which only landed in January, it has a surprising amount of changes landed on top of it, so reverting is actually hard.

I looked at D72372 again, and I have to say the commit message is inscrutable to me (it talks about windows, but this is broken on linux, and about a flag which can only work when set to "on" but is "off" by default???).

But the change itself doesn't look like it can do too much damage. Bernhard confirmed this fixes things on the branch, which sounds good.

For the GitHub issue, you committed D74464 and D74602. Do I understand correctly that those are fixing a problem that's separate from this?

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 26, 2020

There is no dynamic module loading (-load) on Windows, meaning that in the default configuration, Polly was not usable on Windows. This was the first version of the patch.

I should have updated the patch summary. Sorry.

@zmodem
Copy link
Collaborator

@zmodem zmodem commented Feb 26, 2020

There is no dynamic module loading (-load) on Windows, meaning that in the
default configuration, Polly was not usable on Windows. This was the first
version of the patch.

I should have updated the patch summary. Sorry.

Okay, and my second question:

For the GitHub issue, you committed D74464 and D74602. Do I understand
correctly that those are fixing a problem that's separate from this?

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 26, 2020

For the GitHub issue, you committed D74464 and D74602. Do I understand
correctly that those are fixing a problem that's separate from this?

For the GitHub issue, you committed D74464 and D74602. Do I understand
correctly that those are fixing a problem that's separate from this?

I was not involved in these patches. They may cause merge conflicts, but solve different problems. @​sguelton can tell more about them.

The issues are orthogonal. D72372 only changes the default value of LLVM_POLLY_LINK_INTO_TOOLS, depending on whether Polly is on LLVM_ENABLE_PROJECTS.

@zmodem
Copy link
Collaborator

@zmodem zmodem commented Feb 26, 2020

Thanks for clarifying!

I've cherry-picked D72372 to 10.x as d7afdb5.

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 26, 2020

Thanks for clarifying!

I've cherry-picked D72372 to 10.x as
d7afdb5.

Thank you.

They actually don't merge-conflict, I was checking it after writing comment #​11. When submitting the comment, Bugzilla complained because comment #​10 was added while I checked whether D72372 applies cleanly on release/10.0. That's why comment #​11 is messy.

Sorry again for the short notice and hurry. I should have been aware of the issue much earlier and even remarked that we want to keep linking Polly statically by default during the review of an early version of D61446.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants