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

Fix TOSA related breakages #2751

Closed
sdasgup3 opened this issue Mar 19, 2025 · 4 comments · Fixed by #2758
Closed

Fix TOSA related breakages #2751

sdasgup3 opened this issue Mar 19, 2025 · 4 comments · Fixed by #2758
Assignees

Comments

@sdasgup3
Copy link
Member

sdasgup3 commented Mar 19, 2025

Hi @Tai78641

The recent TOSA dialect changes introduced by https://github.com/llvm/llvm-project/pull/129720 have caused breakages, for example:

/usr/local/google/home/sdasgup/Github/stablehlo/stablehlo/conversions/tosa/transforms/TosaRescaleLegalizeToStablehlo.cpp:68:25: error: no member named 'getRoundingMode' in 'mlir::tosa::RescaleOp'                
  bool doubleRound = op.getRoundingMode() == "DOUBLE_ROUND";                                                                                                                                                       
                     ~~ ^                                                                                                                                                                                          
/usr/local/google/home/sdasgup/Github/stablehlo/stablehlo/conversions/tosa/transforms/TosaRescaleLegalizeToStablehlo.cpp:110:9: error: no viable conversion from '::llvm::ArrayRef<int32_t>' (aka 'ArrayRef<int>') 
to 'Value'                                                                                                                                                                                                         
  Value multiplier = op.getMultiplier();   

The PR is an attempt to address some of these issues, but it appears that the problem is more involving than we initially anticipated. Do you mind helping us updating the StablehloQuantLegalizeToTosaRescale.cpp and TosaRescaleLegalizeToStablehlo.cpp passes to accommodate these recent changes.

@sdasgup3
Copy link
Member Author

sdasgup3 commented Mar 19, 2025

#2752 disables certain build and test target to unblock integration while the while issue is fixed.

AI (@sdasgup3 ): Make sure to enable those targets once the current issue is resolved.

sdasgup3 added a commit that referenced this issue Mar 20, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Other than the usual integration, this PR also addresses the followings:

1. Disables certain build and tests for
#2751.

In stablehlo/conversions/tosa/transforms/CMakeLists.txt, just commenting
the problematic files is not enough because

```
 LLVM's build system enforces that all source files are added to a build target
```

Hence we added add `PARTIAL_SOURCES_INTENDED` to the target
specification, though it is discouraged.

1. Fixes `build_tools/integrate/llvm_bump_revision.sh` to account for
unset value of `$1` as enforced by `set -o nounset`
@akuegel
Copy link
Member

akuegel commented Mar 21, 2025

I already have a fix for the compile errors, and adjusted the tests legalize_quant_ops_to_tosa_rescale.mlir and legalize_tosa_rescale_to_stablehlo.mlir
The other disabled tests are still failing and will need adjustments.

@Tai78641
Copy link
Contributor

Tai78641 commented Mar 24, 2025

I pushed a PR: #2756
that fixes the code and tests for the recent tosa dialect updates for TOSA Spec 1.0
note that this includes fixes for rescale zero points changes from attributes to operands in
llvm/llvm-project#130340 that has been merged into llvm but that has not been
pulled into stablehlo's llvm-project integration.

once the stablehlo llvm has been updated to include that, I will test and re-enable tosa legalization in stablehlo

@sdasgup3
Copy link
Member Author

@Tai78641

Thanks again for your fixes!

once the stablehlo llvm has been updated to include that, I will test and re-enable tosa legalization in stablehlo

#2758 is an attempt to test all the affected tests (including nullary.mlir,binary.mlir, and ternay.mlir) after updating the llvm.

This seems working fine and hence closing the ticket. Please feel free to re-open in case there is more to it.

sdasgup3 added a commit that referenced this issue Mar 24, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
fixes #2751
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants