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

Update llvm-project to llvm/llvm-project@01d233ff403823389f848 #3011

Merged

Conversation

hamptonm1
Copy link
Collaborator

LLVM: 01d233ff
StableHLO: 37487a8e

@hamptonm1 hamptonm1 self-assigned this Nov 14, 2024
@@ -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));
Copy link
Collaborator Author

@hamptonm1 hamptonm1 Nov 18, 2024

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.

b().getIntegerAttr(signlessTy, APInt(width, (int64_t)val)));
constant = b().create<arith::ConstantOp>(
loc(), b().getIntegerAttr(signlessTy,
APInt(width, (int64_t)val, false, true)));
Copy link
Contributor

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

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!

Copy link
Contributor

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.

Copy link
Contributor

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!

@@ -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)));
Copy link
Contributor

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.

@hamptonm1
Copy link
Collaborator Author

@AlexandreEichenberger So I went from 5 lit tests failing to now 2. The 2 tests ( onnx-mlir/test/mlir/parallel/krnl_parallel_clause_to_omp.mlir and onnx-mlir/test/mlir/accelerators/nnpa/transform/zlow-stick-unstick-expansion.mlir) that are failing have to do with openmp which I know you are the expert at. After doing some research, it seems like LLVM updated their checks for openmp and as a result the lit tests are failing.

Here is the update:
llvm/llvm-project#112229

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:

within split at /workdir/onnx-mlir/test/mlir/parallel/krnl_parallel_clause_to_omp.mlir:3 offset :22:5: error: 'omp.wsloop' op loop wrapper does not contain exactly one nested op
    omp.wsloop {
    ^

within split at /workdir/onnx-mlir/test/mlir/accelerators/nnpa/transform/zlow-stick-unstick-expansion.mlir:3 offset :7:3: error: expected result type with offset = dynamic instead of 0
  "zlow.stick"(%arg0, %alloc) {layout = "3DS", saturation = -1 : si64} : (memref<16x8x128xf32>, memref<16x8x128xf16, #map>) -> ()
  ^

@hamptonm1 hamptonm1 marked this pull request as ready for review November 18, 2024 15:42
@hamptonm1 hamptonm1 requested a review from mikeessen November 18, 2024 15:42
Copy link
Contributor

@mikeessen mikeessen left a comment

Choose a reason for hiding this comment

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

LGTM!

@hamptonm1
Copy link
Collaborator Author

@AlexandreEichenberger also for s390x, 3 lit tests are failing with one of the same error messages I copied above:

offset :7:3: error: expected result type with offset = dynamic instead of 0

Failed Tests (3):
  Open Neural Network Frontend :: accelerators/nnpa/driver/matmul-div-in-attention-layer.mlir
  Open Neural Network Frontend :: accelerators/nnpa/driver/saturation.mlir
  Open Neural Network Frontend :: accelerators/nnpa/module_op_be/compiler-config.mlir

@AlexandreEichenberger
Copy link
Collaborator

@hamptonm1 The CIs succeed for MacOS, linux on x86, POWER...: does it means that the errors reported for OPenMP:

@AlexandreEichenberger So I went from 5 lit tests failing to now 2. The 2 tests ( onnx-mlir/test/mlir/parallel/krnl_parallel_clause_to_omp.mlir and onnx-mlir/test/mlir/accelerators/nnpa/transform/zlow-stick-unstick-expansion.mlir) that are failing

fails only on s390x?

@hamptonm1
Copy link
Collaborator Author

hamptonm1 commented Dec 5, 2024

@hamptonm1 The CIs succeed for MacOS, linux on x86, POWER...: does it means that the errors reported for OPenMP:

@AlexandreEichenberger So I went from 5 lit tests failing to now 2. The 2 tests ( onnx-mlir/test/mlir/parallel/krnl_parallel_clause_to_omp.mlir and onnx-mlir/test/mlir/accelerators/nnpa/transform/zlow-stick-unstick-expansion.mlir) that are failing

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 macOS job is failing too.

@AlexandreEichenberger
Copy link
Collaborator

@hamptonm1 ok, will look at the PR this week.

@hamptonm1
Copy link
Collaborator Author

@hamptonm1 ok, will look at the PR this week.

@AlexandreEichenberger Thank you and thank you for your patience with me!

@hamptonm1
Copy link
Collaborator Author

@AlexandreEichenberger Thanks for the fix! Can you do the honors of merging the PR for me? Thank you!

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a 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

@AlexandreEichenberger AlexandreEichenberger merged commit b800036 into onnx:main Dec 19, 2024
7 checks passed
@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #16077 [push] Update llvm-project to l... started at 09:00

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #15104 [push] Update llvm-project to l... started at 09:16

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #16075 [push] Update llvm-project to l... started at 08:00

@hamptonm1 hamptonm1 deleted the hamptonm/feature/upgrade-llvm branch December 19, 2024 14:01
@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #16077 [push] Update llvm-project to l... passed after 2 hr 9 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #16075 [push] Update llvm-project to l... passed after 2 hr 38 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #15104 [push] Update llvm-project to l... passed after 3 hr 51 min

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.

5 participants