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

Fix compatibility with LLVM 12 and up #1412

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

luyatshimbalanga
Copy link
Contributor

Description

This pull request addresses a compatibility with LLVM 12 due to the change within LLVM itself. Additionally, this also fix the merge from release to master on #1402

Tests

N/A See https://reviews.llvm.org/D101650

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@lgritz
Copy link
Collaborator

lgritz commented Sep 27, 2021

Can you please amend your commit with the Signed-off-by field to complete the DCO, and then push --force? Thanks.

Upstream LLVM stopped using the compatibility
spellings of OF_{None,Text,Append} from version 12 and up.

https://reviews.llvm.org/D101650

Signed-off-by: Luya Tshimbalanga <[email protected]>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 28, 2021

CLA Signed

The committers are authorized under a signed CLA.

@luyatshimbalanga
Copy link
Contributor Author

Can you please amend your commit with the Signed-off-by field to complete the DCO, and then push --force? Thanks.

Done. It is the first time I noticed the EasyCLA, I don't know if either I or you need to fill the authorization.

@lgritz
Copy link
Collaborator

lgritz commented Sep 28, 2021

The CLA is for you to sign (or your employers, if you work someplace that owns your IP output).

@lgritz
Copy link
Collaborator

lgritz commented Sep 28, 2021

You only have to do the CLA signing once; it will apply to all future PRs to this project from the same github account.

@luyatshimbalanga
Copy link
Contributor Author

You only have to do the CLA signing once; it will apply to all future PRs to this project from the same github account.

Done

@@ -1407,7 +1407,9 @@ LLVM_Util::make_jit_execengine (std::string *err,

options.NoZerosInBSS = false;
options.GuaranteedTailCallOpt = false;
#if OSL_LLVM_VERSION < 120
Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of us had already been building against LLVM 12 with no problem.

Is this maybe supposed to be "130", meaning, it was present in LLVM <= 12 but is missing from >= 13?

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 build failed on Fedora 34 which comes with LLVM 12 hence the condition. It is possible the bug only affected that distribution but I can modify the pull merge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's fine. I was just trying to understand what was going on.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM, as far as the code itself, I am ready to merge it, pending an answer about the question about LLVM 12.

I'm confused about whether this is really fixing compatibility with "LLVM 12 and up" or if it's "LLVM 13 and up." If it's 12, why was everybody else able to build against 12 without any trouble?

@lgritz lgritz merged commit 8682211 into AcademySoftwareFoundation:master Sep 28, 2021
lgritz pushed a commit to lgritz/OpenShadingLanguage that referenced this pull request Oct 24, 2021
Upstream LLVM stopped using the compatibility
spellings of OF_{None,Text,Append} from version 12 and up.

https://reviews.llvm.org/D101650

Signed-off-by: Luya Tshimbalanga <[email protected]>
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.

2 participants