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

[codegen][LLVM][bugfix] Specify argument to FastMathFlags setAllowContract #9337

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

slyubomirsky
Copy link
Contributor

PR #9223 added a fast math flags setting to the LLVM codegen. The setting checks if the LLVM version is >= 6.0 and calls the below lines

    if (fast_math_contract) {
      fmf.setAllowContract();
    }

Recent LLVM versions have a default argument of true to llvm::FastMathFlags::setAllowContract(). However, LLVM 6.0 does not have a default argument for that method, so this fails to compile because it expects an explicit argument. This PR makes the argument explicit to be compatible with LLVM 6.0 (the version my system happened to have).

@slyubomirsky
Copy link
Contributor Author

Please review @masahi (reviewer on PR #9223)

@AndrewZhaoLuo
Copy link
Contributor

AndrewZhaoLuo commented Oct 21, 2021

Can you set the other flags to true explicitly. It appears I missed this in 6.x (I swear they had default args): https://github.com/llvm/llvm-project/blob/release/6.x/llvm/include/llvm/IR/Operator.h

Edit: Actually nvrmind, I cant read, this LGTM

@slyubomirsky
Copy link
Contributor Author

slyubomirsky commented Oct 21, 2021

I can fix others too. I only fixed the one that was breaking my own build locally ;) Wow, how weird, that is literally the only one with an arg

@masahi masahi merged commit 88bf112 into apache:main Oct 21, 2021
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
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.

3 participants