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
Update TargetRewrite after upstreaming #1210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @rovka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great and builds OK for me, thanks.
I don't have permissions to merge this, could someone else please push the button? :) |
auto mod = getModule(); | ||
if (!forcedTargetTriple.empty()) { | ||
setTargetTriple(mod, forcedTargetTriple); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: braces are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not in LLVM, but I think they are in flang:
f18-llvm-project/flang/docs/C++style.md
Line 117 in 1fe8993
Always wrap the bodies of `if()`, `else`, `while()`, `for()`, `do`, &c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files in the Optimizer
directory are actually following the MLIR guidelines (see note in header in this file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Riiight, I completely forgot about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Hard to keep track of the different flavor sometimes :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Clarify some comments as discussed here: flang-compiler#1210
Clarify some comments as discussed here: flang-compiler/f18-llvm-project#1210
Port back some of the changes made to the code while upstreaming TargetRewrite. Note that the upstreaming of TargetRewrite is not yet complete (I'm working on one more patch for AddrOf).