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

hamptonm1 · 2024-11-14T20:14:11Z

LLVM: 01d233ff
StableHLO: 37487a8e

…mptonm1/onnx-mlir into hamptonm/feature/upgrade-llvm

hamptonm1 · 2024-11-18T02:55:43Z

src/Dialect/Mlir/DialectBuilder.cpp

@@ -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));


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.

mikeessen · 2024-11-18T12:32:50Z

src/Dialect/Mlir/DialectBuilder.cpp

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


Please change to c++ style cast to prevent code scan finding.

static_cast<int64_t>(val) instead of (int64_t)val

Also line 637.

@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

@mikeessen There are actually four places that needed to be updated...so I did that for you. Thanks!

@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.

@mikeessen There are actually four places that needed to be updated...so I did that for you. Thanks!

Thank you for updating!

mikeessen · 2024-11-18T12:33:30Z

src/Dialect/Mlir/DialectBuilder.cpp

@@ -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)));


Same comment as previous file. Please change to c++ style cast.

hamptonm1 · 2024-11-18T15:42:38Z

@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>) -> ()
  ^

mikeessen

LGTM!

hamptonm1 · 2024-11-18T20:56:42Z

@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 · 2024-11-22T20:05:23Z

@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?

MegoHam21 added 2 commits November 14, 2024 14:55

Update LLVM and StableHLO

2b95eaf

Add newer version of onnx 1.17.0

33cdacf

hamptonm1 self-assigned this Nov 14, 2024

MegoHam21 and others added 7 commits November 14, 2024 15:17

Fix format

d99d0a8

Merge branch 'main' into hamptonm/feature/upgrade-llvm

d842719

Merge branch 'main' into hamptonm/feature/upgrade-llvm

9c890e9

Update API Int header file

fa9ed4d

Merge branch 'hamptonm/feature/upgrade-llvm' of https://github.com/ha…

e198ed9

…mptonm1/onnx-mlir into hamptonm/feature/upgrade-llvm

Fix format

d13fc9e

Quick fix

9ce6ac1

hamptonm1 commented Nov 18, 2024

View reviewed changes

hamptonm1 requested review from tungld and mikeessen November 18, 2024 02:59

MegoHam21 added 2 commits November 17, 2024 22:07

Try this

4a7036f

Add it back

bc90d56

mikeessen reviewed Nov 18, 2024

View reviewed changes

Remove failing lit tests and address comments

ba873d9

hamptonm1 requested a review from AlexandreEichenberger November 18, 2024 15:42

hamptonm1 marked this pull request as ready for review November 18, 2024 15:42

hamptonm1 requested a review from mikeessen November 18, 2024 15:42

MegoHam21 added 2 commits November 18, 2024 10:44

My fault

ce2cfe6

Mike's feedback

c5d6f9b

mikeessen approved these changes Nov 18, 2024

View reviewed changes

Fix format

c211144

Try fixing up lit tests

e9268d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hamptonm1 commented Nov 14, 2024

hamptonm1 Nov 18, 2024 •

edited

Loading

mikeessen Nov 18, 2024

AlexandreEichenberger Nov 18, 2024

hamptonm1 Nov 18, 2024

mikeessen Nov 18, 2024

mikeessen Nov 18, 2024

mikeessen Nov 18, 2024

hamptonm1 commented Nov 18, 2024

mikeessen left a comment

hamptonm1 commented Nov 18, 2024

AlexandreEichenberger commented Nov 22, 2024

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

Are you sure you want to change the base?

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

Conversation

hamptonm1 commented Nov 14, 2024

hamptonm1 Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

mikeessen Nov 18, 2024

Choose a reason for hiding this comment

AlexandreEichenberger Nov 18, 2024

Choose a reason for hiding this comment

hamptonm1 Nov 18, 2024

Choose a reason for hiding this comment

mikeessen Nov 18, 2024

Choose a reason for hiding this comment

mikeessen Nov 18, 2024

Choose a reason for hiding this comment

mikeessen Nov 18, 2024

Choose a reason for hiding this comment

hamptonm1 commented Nov 18, 2024

mikeessen left a comment

Choose a reason for hiding this comment

hamptonm1 commented Nov 18, 2024

AlexandreEichenberger commented Nov 22, 2024

hamptonm1 Nov 18, 2024 •

edited

Loading