-
Notifications
You must be signed in to change notification settings - Fork 329
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 llvm-project to llvm/llvm-project@01d233ff403823389f848 #3011
Update llvm-project to llvm/llvm-project@01d233ff403823389f848 #3011
Conversation
…mptonm1/onnx-mlir into hamptonm/feature/upgrade-llvm
@@ -695,7 +697,7 @@ TypedAttr MathBuilder::negativeInfAttr(Type type) const { | |||
default: | |||
llvm_unreachable("unsupported element type"); | |||
} | |||
attr = b().getIntegerAttr(type, APInt(width, value)); | |||
attr = b().getIntegerAttr(type, APInt(width, value, false, true)); |
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.
I had to update the APInt
with the following boolean values based on the this PR update: llvm/llvm-project#114539
Previously truncation was set to true as default and they changed it to false, we need the truncation on for our lit tests to pass and to be consistent with the previous behavior.
src/Dialect/Mlir/DialectBuilder.cpp
Outdated
b().getIntegerAttr(signlessTy, APInt(width, (int64_t)val))); | ||
constant = b().create<arith::ConstantOp>( | ||
loc(), b().getIntegerAttr(signlessTy, | ||
APInt(width, (int64_t)val, false, true))); |
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.
Please change to c++ style cast to prevent code scan finding.
static_cast<int64_t>(val) instead of (int64_t)val
Also line 637.
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.
@mikeessen Thanks for keeping track of the casts. Could you look into seeing if there is away to have the compiler issue a warning (probably clang for us since that what we are using)? I am afraid that I may not be as vigilant as we should be on this issue (speaking for me on the code I modify...).
That would be greatly appreciated. Thanks
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.
@mikeessen There are actually four places that needed to be updated...so I did that for you. Thanks!
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.
@mikeessen Thanks for keeping track of the casts. Could you look into seeing if there is away to have the compiler issue a warning (probably clang for us since that what we are using)? I am afraid that I may not be as vigilant as we should be on this issue (speaking for me on the code I modify...).
That would be greatly appreciated. Thanks
Thanks, I will take a look.
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.
@mikeessen There are actually four places that needed to be updated...so I did that for you. Thanks!
Thank you for updating!
src/Dialect/Mlir/DialectBuilder.cpp
Outdated
@@ -2263,7 +2265,8 @@ Value LLVMBuilder::constant(Type type, int64_t val) const { | |||
assert(type.isSignless() && | |||
"LLVM::ConstantOp requires a signless type."); | |||
constant = b().create<LLVM::ConstantOp>(loc(), type, | |||
b().getIntegerAttr(type, APInt(width, (int64_t)val))); | |||
b().getIntegerAttr( | |||
type, APInt(width, (int64_t)val, false, true))); |
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.
Same comment as previous file. Please change to c++ style cast.
@AlexandreEichenberger So I went from 5 lit tests failing to now 2. The 2 tests ( Here is the update: Basically if we have more than one nested omp loop, it emits an error now. So that is why the lit tests are failing. I removed the tests for now but can you do me a favor and take a look. I understand the problem but not sure how to reformat the tests so that they can oblige to the new LLVM rules but also still pass :) errors:
|
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!
@AlexandreEichenberger also for s390x, 3 lit tests are failing with one of the same error messages I copied above:
|
@hamptonm1 The CIs succeed for MacOS, linux on x86, POWER...: does it means that the errors reported for OPenMP:
fails only on s390x? |
Correction, seems like all of the Linux tests are failing once I add back the two failing lit test you requested me to add. Also the |
@hamptonm1 ok, will look at the PR this week. |
@AlexandreEichenberger Thank you and thank you for your patience with me! |
@AlexandreEichenberger Thanks for the fix! Can you do the honors of merging the PR for me? Thank you! |
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 for working on this @hamptonm1 , much appreciated
Jenkins Linux s390x Build #16077 [push] Update llvm-project to l... started at 09:00 |
Jenkins Linux ppc64le Build #15104 [push] Update llvm-project to l... started at 09:16 |
Jenkins Linux amd64 Build #16075 [push] Update llvm-project to l... started at 08:00 |
Jenkins Linux s390x Build #16077 [push] Update llvm-project to l... passed after 2 hr 9 min |
Jenkins Linux amd64 Build #16075 [push] Update llvm-project to l... passed after 2 hr 38 min |
Jenkins Linux ppc64le Build #15104 [push] Update llvm-project to l... passed after 3 hr 51 min |
LLVM:
01d233ff
StableHLO:
37487a8e