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

Handle default inlining on funcOp and callOp #19498

Open
raikonenfnu opened this issue Dec 17, 2024 · 7 comments
Open

Handle default inlining on funcOp and callOp #19498

raikonenfnu opened this issue Dec 17, 2024 · 7 comments

Comments

@raikonenfnu
Copy link
Collaborator

After llvm/llvm-project#119970 landed upstream, we are running issues compiling some of our models. It throws off:

_ IREE compile and run: open-llama-3b-v2-f16::open-llama-3b-v2-f16.mlirbc::open-llama-3b-v2-f16.mlirbc::gpu_rocm::real_weights_prefill _
Error invoking iree-compile
Error code: 1
Stderr diagnostics:
open-llama-3b-v2-f16.mlirbc:0:0: error: attempting to parse a byte at the end of the bytecode
open-llama-3b-v2-f16.mlirbc:0:0: note: in bytecode version 6 produced by: MLIR19.0.0git


/home/esaimana/iree_tests_cache/artifacts/sd3_vae/model.mlirbc:0:0: error: attempting to parse a byte at the end of ...esaimana/iree_tests_cache/artifacts/sd3_vae/model.mlirbc:0:0: note: in bytecode version 6 produced by: MLIR19.0.0git

error: expected mlir::ArrayAttr, but got: "__auto.t5...ome/nod/iree_tests_cache/artifacts/sd3_clip/model.mlirbc:0:0: note: in bytecode version 6 produced by: MLIR19.0.0git

A good way to repro this issue is to do:

wget https://sharkpublic.blob.core.windows.net/sharkpublic/sai/sd3-mmdit/model.mlirbc -O mmdit.mlirbc
/path/to/iree-opt mmdit.mlirbc -o /dev/null

expected output looks something like:
error: expected mlir::ArrayAttr, but got: "__auto.t5...ome/nod/iree_tests_cache/artifacts/sd3_clip/model.mlirbc:0:0: note: in bytecode version 6 produced by: MLIR19.0.0git

wget https://sharkpublic.blob.core.windows.net/sharkpublic/sai/sdxl-scheduled-unet/model.mlirbc
/path/to/iree-opt mmdit.mlirbc -o /dev/null

Expected output looks something like:
attempting to parse a byte at the end of note: in bytecode version 6 produced by: MLIR19.0.0git
raikonenfnu added a commit that referenced this issue Dec 17, 2024
Update LLVM to llvm/llvm-project@3f136f7
(#19479)
Carrying the following reverts

- llvm/llvm-project#116470
- llvm/llvm-project#117424
- llvm/llvm-project#119671
- llvm/llvm-project#119970

First two are carry over from previous-previous integrate. It is being
fixed in
#19451 . The last one is a from the
previous integrate.
The last one is a new error being tracked in
#19498

---------

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 17, 2024
Update LLVM to llvm/llvm-project@b07e7b76c5d532a6 (llvm/llvm-project#120002)
Carrying the following reverts

- llvm/llvm-project#116470
- llvm/llvm-project#117424
- llvm/llvm-project#119671
- llvm/llvm-project#119970

First two are carry over from previous-previous integrate. It is being fixed in
iree-org#19451 . The last one is a from the previous integrate.
The last one is a new error being tracked in iree-org#19498

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 17, 2024
Update LLVM to llvm/llvm-project@b07e7b76c5d532a6 (llvm/llvm-project#120002)
Carrying the following reverts

- llvm/llvm-project#116470
- llvm/llvm-project#117424
- llvm/llvm-project#119671
- llvm/llvm-project#119970

First two are carry over from previous-previous integrate. It is being fixed in
iree-org#19451 . The last one is a from the previous integrate.
The last one is a new error being tracked in iree-org#19498

Signed-off-by: Stanley Winata <[email protected]>
@ScottTodd
Copy link
Member

I find it strange that adding a new optional attribute broke parsing, but only for MLIR bytecode and not MLIR text. Is that the expected behavior?


Sample logs: https://github.com/iree-org/iree/actions/runs/12358432483

We can update our tests as needed... and this could be a good forcing function.

@ScottTodd
Copy link
Member

Sent nod-ai/SHARK-TestSuite#417 and nod-ai/SHARK-TestSuite#418 to delete some old tests that are (almost) redundant at this point.

@raikonenfnu
Copy link
Collaborator Author

Cross posting from our convo on discord:

looks like the root issue It is to do with the inline attribute(per the reverted commit), i.e we'd need the util.globalOp to have that noinline attribute on i.e:

  util.global private @_params.unet.time_embedding.linear_1.bias {noinline} = #stream.parameter.named<"model"::"unet.time_embedding.linear_1.bias"> : tensor<1280xf16>
  util.global private @_params.unet.time_embedding.linear_2.weight {noinline} = #stream.parameter.named<"model"::"unet.time_embedding.linear_2.weight"> : tensor<1280x1280xf16>
  util.global private @_params.unet.time_embedding.linear_2.bias {noinline} = #stream.parameter.named<"model"::"unet.time_embedding.linear_2.bias"> : tensor<1280xf16>
  util.global private @_params.unet.add_embedding.linear_1.weight {noinline} = #stream.parameter.named<"model"::"unet.add_embedding.linear_1.wei

I think we just need to regenerate the IR without modifying any of the code and should generate the {noinline} attribute now by default! :slight_smile:

I tested the hypothesis above locally, i.e:

(with reverted noinline) iree-compile model.mlirbc --compile-to=input -o model_input.mlir (the IR generated her has {noinline} autogenerated)

(with unrevert the noinline patch) iree-opt model.mlirbc -> same error
(with unrevert the noinline patch) iree-opt model_input.mlir -> works!

@raikonenfnu
Copy link
Collaborator Author

I find it strange that adding a new optional attribute broke parsing, but only for MLIR bytecode and not MLIR text. Is that the expected behavior?

Not familiar with these side of the stack so not sure also, all I know is bytecode == smaller memory footprint + non-readable. 😅

The regeneration process is not documented for any of those files and they haven't been updated since they were added 5 months ago.

Yeah that's unfortunate, what do you think if I just do an iree-opt on it to get some noinline attributes on it and then convert it to back to an mlirbc? Or would that be too hacky?

@benvanik
Copy link
Collaborator

util.global should use inlining_policy, e.g.

util.global private @_params.classifier.weight {inlining_policy = #util.inline.never} : tensor<30x20xf32>

@raikonenfnu
Copy link
Collaborator Author

util.global should use inlining_policy

Ah interesting, need to figure out if this gets autogenerated now from sharktank (unless here knows). But in that case, we may need to either:

a) regenerate with correct inlining policy (a bit more challenging because regeneration process is not documented)
or
b) iree-opt to get text, insert it with some regex, and re-encode it back to mlirbc

LMK if you guys have a preference on which solution works better, or if there' something else we can do to resolve this

raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 20, 2024
Changes in C++ mostly handle changes done in llvm/llvm-project@4b56345 that combines SCFTilingResult and SCFReductionTilingResult.

This PR also carries the following reverts

- llvm/llvm-project#119671
- llvm/llvm-project#119970

The first one is a from the previous integrate. The last one is a new error being tracked in iree-org#19498.

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 20, 2024
Changes in C++ mostly handle changes done in llvm/llvm-project@4b56345 that combines SCFTilingResult and SCFReductionTilingResult.

This PR also carries the following reverts

- llvm/llvm-project#119671
- llvm/llvm-project#119970

The first one is a from the previous integrate. The last one is a new error being tracked in iree-org#19498.

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 20, 2024
Changes in C++ mostly handle changes done in llvm/llvm-project@4b56345 that combines SCFTilingResult and SCFReductionTilingResult.

This PR also carries the following reverts

- llvm/llvm-project#119970

The issue related to the revert is being tracked in iree-org#19498.

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 23, 2024
Update LLVM to llvm/llvm-project@b13592219c421820b (llvm/llvm-project#85376)
Changes done to resolve mlirbc issue in iree-org#19498

This PR also carries the following reverts:

llvm/llvm-project#120115

The main issue with this PR is it breaks matvec codegen generating
scf.if instead of scf.for(s). An issue will be pushed up for repro.

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 23, 2024
Update LLVM to llvm/llvm-project@b13592219c421820b (llvm/llvm-project#85376)
Changes done to resolve mlirbc issue in iree-org#19498

This PR also carries the following reverts:

llvm/llvm-project#120115

The main issue with this PR is it breaks matvec codegen generating
scf.if instead of scf.for(s). An issue will be pushed up for repro.

Signed-off-by: Stanley Winata <[email protected]>
@raikonenfnu
Copy link
Collaborator Author

Hey @ScottTodd, fyi I have regenerated the MLIR and uploaded them to the Azure, plus regenerated the mlirbc in SHARK-TestSuite to the latest version here https://github.com/nod-ai/SHARK-TestSuite/tree/raikonen/integrate/llvm20241220.

raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 25, 2024
Update LLVM to llvm/llvm-project@b13592219c421820b (llvm/llvm-project#85376)
Changes done to resolve mlirbc issue in iree-org#19498

This PR also carries the following reverts:

llvm/llvm-project#120115

The main issue with this PR is it breaks matvec codegen generating
scf.if instead of scf.for(s). An issue will be pushed up for repro.

Signed-off-by: Stanley Winata <[email protected]>
raikonenfnu added a commit to raikonenfnu/iree that referenced this issue Dec 26, 2024
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

No branches or pull requests

3 participants